Messages in this thread |  | | Date | Sat, 28 Dec 2019 23:40:39 +0800 | From | Gen Zhang <> | Subject | Re: [PATCH] board-dm644x-evm: fix 2 missing-check bugs in evm_led_setup() |
| |
On Sat, Dec 28, 2019 at 01:48:24PM +0000, Russell King - ARM Linux admin wrote: > This is the old everything-successful path through the code: > > platform_device_alloc() > platform_device_add_data() > platform_device_add() > evm_led_dev is set to the device > > This is the new everything-successful path through the code: > > platform_device_alloc() > platform_device_add_data() > platform_device_add() > platform_device_put() > evm_led_dev = NULL Thanks for your reply. Adding a return may handle this. successful path: platform_device_alloc() platform_device_add_data() platform_device_add() return status error path: platform_device_put() evm_led_dev = NULL return status correct?
> > And, specifically, the code sequence (I quote from your patch): > > if (status) > goto err; > err: > > is very stupid; it might as well not exist at all.
Well, you have to admit that the result of platform_device_alloc(), platform_device_add_data() and platform_device_add() should be checked, and error should be handled.
e.g., there are 124 call sites of platform_device_add_data() in Linux. Only 24 are not checked, and most of them are in arm subsystem.
Look forward to deeper discussion of this problem.
Best, Gen > > Since other code references evm_led_dev, one can assume that we do > not want it to be NULL for the success path. So, taking all this > together, your patch is very very wrong, and I also find it very > worrying too. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up
|  |