* Need help determining if the change is warranted. @ 2023-03-27 11:34 Anton Gusev 2023-05-27 0:36 ` Alison Schofield 0 siblings, 1 reply; 4+ messages in thread From: Anton Gusev @ 2023-03-27 11:34 UTC (permalink / raw) To: kernelnewbies In the file drivers/leds/flash/leds-lm3601x.c, function lm3601x_strobe_set, the calls to regmap_update_bits aren't checked for errors. I am unsure whether adding the checks is warranted, since lm3601x_read_faults might cover the conditions that can cause regmap_update_bits to fail there. On the other hand, if this is not true, then lm3601x_strobe_set can fail silently. Also, all other calls to regmap_update_bit in the driver are checked or directly returned. -- Anton Gusev _______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Need help determining if the change is warranted. 2023-03-27 11:34 Need help determining if the change is warranted Anton Gusev @ 2023-05-27 0:36 ` Alison Schofield 2023-05-27 6:05 ` Lucas Tanure 0 siblings, 1 reply; 4+ messages in thread From: Alison Schofield @ 2023-05-27 0:36 UTC (permalink / raw) To: Anton Gusev; +Cc: kernelnewbies This may be a resend. My first msg may be stuck in moderation, because I sent w a new email addr. On Mon, Mar 27, 2023 at 02:34:33PM +0300, Anton Gusev wrote: > In the file drivers/leds/flash/leds-lm3601x.c, function lm3601x_strobe_set, > the calls to regmap_update_bits aren't checked for errors. > > I am unsure whether adding the checks is warranted, since > lm3601x_read_faults might cover the conditions that can cause > regmap_update_bits to fail there. On the other hand, if this is not true, > then lm3601x_strobe_set can fail silently. Also, all other calls to > regmap_update_bit in the driver are checked or directly returned. > -- > Anton Gusev Hi Anton, I don't see a patch posted for this, so I'll go ahead and respond. It seems *something* can be improved here. Either examine those ret values as they roll in, or stop assigning those ret values. Maybe, as you guess, lm3601x_read_faults() is doing the needed checks. That's something you could dig into further before posting the patch. Also, note the answer might not be the same for all 3 of those calls. Alison > > _______________________________________________ > Kernelnewbies mailing list > Kernelnewbies@kernelnewbies.org > https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies _______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Need help determining if the change is warranted. 2023-05-27 0:36 ` Alison Schofield @ 2023-05-27 6:05 ` Lucas Tanure 2023-05-28 4:18 ` Valdis Klētnieks 0 siblings, 1 reply; 4+ messages in thread From: Lucas Tanure @ 2023-05-27 6:05 UTC (permalink / raw) To: Alison Schofield; +Cc: Anton Gusev, kernelnewbies On Sat, May 27, 2023 at 1:37 AM Alison Schofield <alison.schofield@intel.com> wrote: > > This may be a resend. My first msg may be stuck in moderation, > because I sent w a new email addr. > > On Mon, Mar 27, 2023 at 02:34:33PM +0300, Anton Gusev wrote: > > In the file drivers/leds/flash/leds-lm3601x.c, function lm3601x_strobe_set, > > the calls to regmap_update_bits aren't checked for errors. > > > > I am unsure whether adding the checks is warranted, since > > lm3601x_read_faults might cover the conditions that can cause > > regmap_update_bits to fail there. On the other hand, if this is not true, > > then lm3601x_strobe_set can fail silently. Also, all other calls to > > regmap_update_bit in the driver are checked or directly returned. > > -- > > Anton Gusev > > Hi Anton, > > I don't see a patch posted for this, so I'll go ahead and respond. > > It seems *something* can be improved here. Either examine those ret values > as they roll in, or stop assigning those ret values. Maybe, as you guess, > lm3601x_read_faults() is doing the needed checks. That's something you > could dig into further before posting the patch. Also, note the answer > might not be the same for all 3 of those calls. > > Alison > > > > > _______________________________________________ > > Kernelnewbies mailing list > > Kernelnewbies@kernelnewbies.org > > https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies > > _______________________________________________ > Kernelnewbies mailing list > Kernelnewbies@kernelnewbies.org > https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies It's unusual for an I2C bus would suddenly stop working, so just one check at the beginning of the function is enough. I would remove all ret assignments apart from the first one for every function on that driver. _______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Need help determining if the change is warranted. 2023-05-27 6:05 ` Lucas Tanure @ 2023-05-28 4:18 ` Valdis Klētnieks 0 siblings, 0 replies; 4+ messages in thread From: Valdis Klētnieks @ 2023-05-28 4:18 UTC (permalink / raw) To: tanure; +Cc: Alison Schofield, Anton Gusev, kernelnewbies [-- Attachment #1.1: Type: text/plain, Size: 1186 bytes --] On Sat, 27 May 2023 07:05:40 +0100, Lucas Tanure said: > It's unusual for an I2C bus would suddenly stop working, so just one > check at the beginning of the function is enough. > I would remove all ret assignments apart from the first one for every > function on that driver. By the same token, it's somewhat unusual for an I2C bus to stop working once it's been probed and initialized, so maybe the first one is superfluous as well :) (Just playing devil's advocate here - after 4 decades of this stuff, I've hit enough systems that have gotten wedged on unusual situations. The worst one was an 18 month chase for why on occasion a petabyte-scale disk array hanging off a bunch of FiberChannel connections would read from the wrong LUN. We finally figured out it was actually a firmware bug in the 10Gig ethernet card. But probably the best advice is this one variously attributed to Tom Duff at Bell Labs (yeah the guy who came up with the Duff Device) or Henry Spencer: Never test for an error condition you don't know how to handle.... (And that 10gig issue probably needed Casey Schaufler of SGI's suggested error code: -EGADS - Violates the principle of least surprise. [-- Attachment #1.2: Type: application/pgp-signature, Size: 494 bytes --] [-- Attachment #2: Type: text/plain, Size: 170 bytes --] _______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-28 4:20 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-27 11:34 Need help determining if the change is warranted Anton Gusev 2023-05-27 0:36 ` Alison Schofield 2023-05-27 6:05 ` Lucas Tanure 2023-05-28 4:18 ` Valdis Klētnieks
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).