Messages in this thread | | | Date | Mon, 15 Aug 2022 14:53:07 -0600 | From | Jack Rosenthal <> | Subject | Re: [PATCH v8] firmware: google: Implement cbmem in sysfs driver |
| |
Thanks for the detailed review!
On 2022-08-09 at 20:49 -0500, Stephen Boyd wrote: > /sys/firmware/coreboot/cbmem?
Fixed in v9.
> > +#include <linux/ctype.h> > > What is used from this header?
Not used, fixed in v9.
> Are all entries supposed to be writeable? Are there entries that are > read-only?
The idea here was that we could use crossystem to update the cbmem entry when it makes changes, however, I do see this as an odd usage of the ABI, and in v9, I removed writing entirely and all entries are now read-only.
If tools like crossystem need to maintain changes to this buffer, they should either maintain that copy themselves in /run or something, or daemonize and keep it in their process' memory.
> > + return cbmem_entry_setup(entry); > > Is there a reason this isn't inlined here?
cbmem_entry_setup was already a little long, so I just did this for reading clarity. If you think it would be clearer just inlined here, I can change it (let me know).
> Need to set .owner = THIS_MODULE unless you use > module_coreboot_driver().
Done.
> > + if (!coreboot_kobj) > > cbmem_kobj?
Done.
| |