| Message ID | 1624056311-6836-1-git-send-email-gabeknez@linux.microsoft.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpiolib: cdev: zero padding during conversion to gpioline_info_changed | expand |
On Fri, Jun 18, 2021 at 03:45:11PM -0700, Gabriel Knezek wrote: Probably should've been [PATCH v2], despite the subject rename. And CC the maintainers (LinusW and Bart here) and past reviewers (Andy and me). > From: Gabriel Knezek <gabeknez@microsoft.com> > A second From: header? With a different address? Perhaps you could pick one? Neither git nor checkpatch.pl seem to mind, but it is odd. > When userspace requests a GPIO v1 line info changed event, the kernel > populates and returns the gpioline_info_changed structure. That structure > contains 5 words of padding at the end of the structure that are not > initialized before being returned to usermode: > Avoid code snippets in checkin comments - try to stick to plain English. So replace "the kernel" with "lineinfo_watch_read()" and drop the snippets. And "usermode" -> "userspace". > struct gpioline_info_changed { > struct gpioline_info info; > __u64 timestamp; > __u32 event_type; > __u32 padding[5]; /* for future use */ > }; > > Which is used here in the lineinfo_watch_read routine: > } else { > struct gpioline_info_changed event_v1; > gpio_v2_line_info_changed_to_v1(&event, &event_v1); > if (copy_to_user(buf + bytes_read, &event_v1, > event_size)) > return -EFAULT; > > Fix this by zeroing the structure in gpio_v2_line_info_change_to_v1 > before populating its contents. > Make that "gpio_v2_line_info_change_to_v1()" as you are referring to a function. And maybe "Fix this by zeroing" to just "Zero"? > Signed-off-by: Gabriel Knezek <gabeknez@microsoft.com> You should retain the Fixes tag from v1 - it is important to identify where this patch will need to be backported to. And include at least the first twelve characters of the SHA-1 [1]. > --- > drivers/gpio/gpiolib-cdev.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index ee5903aac497..af68532835fe 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -1865,6 +1865,7 @@ static void gpio_v2_line_info_changed_to_v1( > struct gpio_v2_line_info_changed *lic_v2, > struct gpioline_info_changed *lic_v1) > { > + memset(lic_v1, 0, sizeof(*lic_v1)); > gpio_v2_line_info_to_v1(&lic_v2->info, &lic_v1->info); > lic_v1->timestamp = lic_v2->timestamp_ns; > lic_v1->event_type = lic_v2->event_type; > -- > 2.25.1 > Still good with this bit ;) Cheers, Kent. [1] https://www.kernel.org/doc/html/v5.12/process/submitting-patches.html
On Sat, Jun 19, 2021 at 12:11:44PM +0800, Kent Gibson wrote: > On Fri, Jun 18, 2021 at 03:45:11PM -0700, Gabriel Knezek wrote: > > Probably should've been [PATCH v2], despite the subject rename. > > And CC the maintainers (LinusW and Bart here) and past reviewers (Andy > and me). > Sorry about that. When sending the revised patch, should I change it to v2? Or perhaps v3? > > From: Gabriel Knezek <gabeknez@microsoft.com> > > > > A second From: header? With a different address? > Perhaps you could pick one? > Neither git nor checkpatch.pl seem to mind, but it is odd. > Apologies for that as well. git send-email doesn't play well with Exchange, so after talking to coworkers it turns out we have a separate email server for sending patches which I wasn't aware of before I sent the first one. :-/ Sorry for the confusion; I'll stick to the @linux address. > > > Signed-off-by: Gabriel Knezek <gabeknez@microsoft.com> > > You should retain the Fixes tag from v1 - it is important to identify > where this patch will need to be backported to. > And include at least the first twelve characters of the SHA-1 [1]. > Oh shoot. Sorry about that as well. I need to make a checklist. Thanks for the link; I did read it before submitting the second patch, but I clearly missed that part. > > Still good with this bit ;) > > Cheers, > Kent. Thanks for the kind help. I promise I'll update our wiki with these learnings so hopefully dealing with the next person from our team submitting a patch won't be so painful. -Gabe
On Sat, Jun 19, 2021 at 8:49 PM Gabriel Knezek <gabeknez@linux.microsoft.com> wrote: > On Sat, Jun 19, 2021 at 12:11:44PM +0800, Kent Gibson wrote: > > On Fri, Jun 18, 2021 at 03:45:11PM -0700, Gabriel Knezek wrote: > > > > Probably should've been [PATCH v2], despite the subject rename. > > > > And CC the maintainers (LinusW and Bart here) and past reviewers (Andy > > and me). > Sorry about that. When sending the revised patch, should I change it to v2? > Or perhaps v3? Yes, v3 with a changelog for each previous version. ... > > > From: Gabriel Knezek <gabeknez@microsoft.com> > > > > > > > A second From: header? With a different address? > > Perhaps you could pick one? > > Neither git nor checkpatch.pl seem to mind, but it is odd. > > Apologies for that as well. git send-email doesn't play well with Exchange, so > after talking to coworkers it turns out we have a separate email server for > sending patches which I wasn't aware of before I sent the first one. :-/ Sorry > for the confusion; I'll stick to the @linux address. Don't forget to configure your Git accordingly (globally or per this project). Note, you always can test this by doing git send-email --suppress-cc=all --to "Your Second <email@address>" or so. > > > Signed-off-by: Gabriel Knezek <gabeknez@microsoft.com> > > > > You should retain the Fixes tag from v1 - it is important to identify > > where this patch will need to be backported to. > > And include at least the first twelve characters of the SHA-1 [1]. > > Oh shoot. Sorry about that as well. I need to make a checklist. The checklist is already made for you: https://www.kernel.org/doc/html/latest/process/submit-checklist.html > Thanks for the link; I did read it before submitting the second patch, > but I clearly missed that part. > > > Still good with this bit ;) > Thanks for the kind help. I promise I'll update our wiki with these > learnings so hopefully dealing with the next person from our team submitting > a patch won't be so painful. -- With Best Regards, Andy Shevchenko
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index ee5903aac497..af68532835fe 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -1865,6 +1865,7 @@ static void gpio_v2_line_info_changed_to_v1( struct gpio_v2_line_info_changed *lic_v2, struct gpioline_info_changed *lic_v1) { + memset(lic_v1, 0, sizeof(*lic_v1)); gpio_v2_line_info_to_v1(&lic_v2->info, &lic_v1->info); lic_v1->timestamp = lic_v2->timestamp_ns; lic_v1->event_type = lic_v2->event_type;