| Message ID | 559D8874.7090407@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
This one deserves some extra review because it introduces a call to: WARN_ON(extra_checks); in gpiod_free(). That may or may not matter... regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 8 Jul 2015, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 8 Jul 2015 22:12:25 +0200 > > The gpiod_put() function performs also input parameter validation > by forwarding its single input pointer to the gpiod_free() function. > Thus the test around the calls is not needed. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> As Dan correctly pointed out, this is not as straightforward as it might seem on a firsr sight, because there is a WARN_ON() that might start triggering in case of !ihid->desc. Adding Benjamin. I am not applying this without his Ack.
On Thu, Jul 9, 2015 at 8:34 AM, Jiri Kosina <jkosina@suse.com> wrote: > On Wed, 8 Jul 2015, SF Markus Elfring wrote: > >> From: Markus Elfring <elfring@users.sourceforge.net> >> Date: Wed, 8 Jul 2015 22:12:25 +0200 >> >> The gpiod_put() function performs also input parameter validation >> by forwarding its single input pointer to the gpiod_free() function. >> Thus the test around the calls is not needed. >> >> This issue was detected by using the Coccinelle software. >> >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > > As Dan correctly pointed out, this is not as straightforward as it might > seem on a firsr sight, because there is a WARN_ON() that might start > triggering in case of !ihid->desc. > > Adding Benjamin. I am not applying this without his Ack. > I think the gpiod case is the exception rather than the common rule (most i2c-hid device we saw until recently were using irqs, not gpios). So if I understand correctly, removing the check on ihid->desc would raise a warning for most devices. This is IMO not a good thing, so I would say NACK. Mika might have a different opinion though. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> The gpiod_put() function performs also input parameter validation >>> by forwarding its single input pointer to the gpiod_free() function. >>> Thus the test around the calls is not needed. >>> >>> This issue was detected by using the Coccinelle software. >>> >>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >> >> As Dan correctly pointed out, this is not as straightforward as it might >> seem on a firsr sight, because there is a WARN_ON() that might start >> triggering in case of !ihid->desc. >> >> Adding Benjamin. I am not applying this without his Ack. >> > > I think the gpiod case is the exception rather than the common rule > (most i2c-hid device we saw until recently were using irqs, not > gpios). So if I understand correctly, removing the check on ihid->desc > would raise a warning for most devices. This is IMO not a good thing, > so I would say NACK. > > Mika might have a different opinion though. The proposed update candidates are contained in the source file "drivers/hid/i2c-hid/i2c-hid.c" from Linux next-20150708. * i2c_hid_remove() function: Can it be tolerated here that the pointer "ihid->desc" might be eventually null? * i2c_hid_probe() function: Is this implementation structured in such a way that a pointer for valid data will be usually passed for "ihid->desc" if the statements after the jump label "err" will be reached? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 9, 2015 at 4:49 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>>> The gpiod_put() function performs also input parameter validation >>>> by forwarding its single input pointer to the gpiod_free() function. >>>> Thus the test around the calls is not needed. >>>> >>>> This issue was detected by using the Coccinelle software. >>>> >>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >>> >>> As Dan correctly pointed out, this is not as straightforward as it might >>> seem on a firsr sight, because there is a WARN_ON() that might start >>> triggering in case of !ihid->desc. >>> >>> Adding Benjamin. I am not applying this without his Ack. >>> >> >> I think the gpiod case is the exception rather than the common rule >> (most i2c-hid device we saw until recently were using irqs, not >> gpios). So if I understand correctly, removing the check on ihid->desc >> would raise a warning for most devices. This is IMO not a good thing, >> so I would say NACK. >> >> Mika might have a different opinion though. > > The proposed update candidates are contained in the source > file "drivers/hid/i2c-hid/i2c-hid.c" from Linux next-20150708. > > * i2c_hid_remove() function: > Can it be tolerated here that the pointer "ihid->desc" might be eventually null? > > * i2c_hid_probe() function: > Is this implementation structured in such a way that a pointer for valid data > will be usually passed for "ihid->desc" if the statements after the jump > label "err" will be reached? > Again, in both case it is completely normal to have "ihid->desc == NULL" given that this field is only retrieved in case of an ACPI device which does not declares an IRQ but a GPIO. Most ACPI devices I saw are using a simple IRQ, and the OF instantiations of the driver will definitively have ihid->desc null. So I do not want to have a warning for most of i2c-hid devices out there (because I will have to explain that this is completely normal again and again). Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> The proposed update candidates are contained in the source >> file "drivers/hid/i2c-hid/i2c-hid.c" from Linux next-20150708. >> >> * i2c_hid_remove() function: >> Can it be tolerated here that the pointer "ihid->desc" might be eventually null? >> >> * i2c_hid_probe() function: >> Is this implementation structured in such a way that a pointer for valid data >> will be usually passed for "ihid->desc" if the statements after the jump >> label "err" will be reached? >> > > Again, in both case it is completely normal to have "ihid->desc == > NULL" given that this field is only retrieved in case of an ACPI > device which does not declares an IRQ but a GPIO. Most ACPI devices I > saw are using a simple IRQ, and the OF instantiations of the driver > will definitively have ihid->desc null. So I do not want to have a > warning for most of i2c-hid devices out there (because I will have to > explain that this is completely normal again and again). Would it make sense to annotate checks before such function calls as "unlikely" then? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 9, 2015 at 5:10 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>> The proposed update candidates are contained in the source >>> file "drivers/hid/i2c-hid/i2c-hid.c" from Linux next-20150708. >>> >>> * i2c_hid_remove() function: >>> Can it be tolerated here that the pointer "ihid->desc" might be eventually null? >>> >>> * i2c_hid_probe() function: >>> Is this implementation structured in such a way that a pointer for valid data >>> will be usually passed for "ihid->desc" if the statements after the jump >>> label "err" will be reached? >>> >> >> Again, in both case it is completely normal to have "ihid->desc == >> NULL" given that this field is only retrieved in case of an ACPI >> device which does not declares an IRQ but a GPIO. Most ACPI devices I >> saw are using a simple IRQ, and the OF instantiations of the driver >> will definitively have ihid->desc null. So I do not want to have a >> warning for most of i2c-hid devices out there (because I will have to >> explain that this is completely normal again and again). > > Would it make sense to annotate checks before such function calls > as "unlikely" then? > I don't see the benefits of this right now. These calls are not time critical and it's not because today they are few devices with GPIOs rather than IRQs that it will be the case all the time. You can always submit a patch preventing coccinelle to raise this warning, but maybe a simple comment in the code would be enough. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index f77469d..09ff4e7 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -1048,9 +1048,7 @@ err_pm: pm_runtime_disable(&client->dev); err: - if (ihid->desc) - gpiod_put(ihid->desc); - + gpiod_put(ihid->desc); i2c_hid_free_buffers(ihid); kfree(ihid); return ret; @@ -1074,9 +1072,7 @@ static int i2c_hid_remove(struct i2c_client *client) if (ihid->bufsize) i2c_hid_free_buffers(ihid); - if (ihid->desc) - gpiod_put(ihid->desc); - + gpiod_put(ihid->desc); kfree(ihid); acpi_dev_remove_driver_gpios(ACPI_COMPANION(&client->dev));