| Message ID | b7e69e04-e15c-41ec-b62b-37253debc654@web.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powermac: Call of_node_put(bk_node) only once in pmac_has_backlight_type() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 5 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 21 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
First of all, the change is wrong. We can't dereference "prop" after calling of_node_put(). You have to be a bit extra careful reviewing Markus's patches because a lot of the rest of us have blocked these messages so you're on your own in that way. On Wed, Oct 02, 2024 at 10:43:46PM +0200, Christophe Leroy wrote: > > > Le 02/10/2024 à 22:02, Markus Elfring a écrit : > > From: Markus Elfring <elfring@users.sourceforge.net> > > Date: Wed, 2 Oct 2024 21:50:27 +0200 > > > > An of_node_put(bk_node) call was immediately used after a pointer check > > for an of_get_property() call in this function implementation. > > Thus call such a function only once instead directly before the check. > > It seems pointless to perform a put immediately after a get. Shouldn't > of_find_property() be used instead ? And then of_property_read_string() > would probably be better. > > Maybe you can even use of_property_match_string(). The of_get_property() function doesn't do a get as in get/put, it just finds the property and returns it. It doesn't bump the reference count. It's a confusing name in that way. The The of_node_put() pairs with of_find_node_by_name(). regards, dan carpenter
On 03/10/2024 07:56, Dan Carpenter wrote: > First of all, the change is wrong. We can't dereference "prop" after calling > of_node_put(). You have to be a bit extra careful reviewing Markus's patches > because a lot of the rest of us have blocked these messages so you're on your > own in that way. Yep, I plonked him some time ago and everything is in spam. The code looks just incorrect and I think Markus did not understand it before transforming. Best regards, Krzysztof
> First of all, the change is wrong. We can't dereference "prop" after calling > of_node_put(). … > The of_get_property() function doesn't do a get as in get/put, it just finds > the property and returns it. It doesn't bump the reference count. It's a > confusing name in that way. The The of_node_put() pairs with > of_find_node_by_name(). Thanks for your information. * Do you see opportunities for improving the software documentation accordingly? * How much can source code analysis tools influence development efforts? Regards, Markus
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 02/10/2024 à 22:02, Markus Elfring a écrit : >> From: Markus Elfring <elfring@users.sourceforge.net> >> Date: Wed, 2 Oct 2024 21:50:27 +0200 >> >> An of_node_put(bk_node) call was immediately used after a pointer check >> for an of_get_property() call in this function implementation. >> Thus call such a function only once instead directly before the check. > > It seems pointless to perform a put immediately after a get. Shouldn't > of_find_property() be used instead ? And then of_property_read_string() > would probably be better. > > Maybe you can even use of_property_match_string(). Yes that would clean it up nicely I think, eg: int pmac_has_backlight_type(const char *type) { struct device_node* bk_node = of_find_node_by_name(NULL, "backlight"); int i; i = of_property_match_string(bk_node, "backlight-control", type); of_node_put(bk_node); return i >= 0; } cheers
Le 11/10/2024 à 18:18, Markus Elfring a écrit : > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 11 Oct 2024 18:10:06 +0200 > > Replace an of_get_property() call by of_property_match_string() > so that this function implementation can be simplified. > > Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu> > Link: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinuxppc-dev%2Fd9bdc1b6-ea7e-47aa-80aa-02ae649abf72%40csgroup.eu%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cf278e44683c04b931b9c08dcea106447%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638642603333398766%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=6byvgvuGiBSVu8F6kLA2OozUuHZunJRH%2BU%2Bq9q7osmM%3D&reserved=0 > Suggested-by: Michael Ellerman <mpe@ellerman.id.au> > Link: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinuxppc-dev%2F87cyk97ufp.fsf%40mail.lhotse%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cf278e44683c04b931b9c08dcea106447%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638642603333422636%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=IDuYfe3UoaIEmedJ07H67zvzrPnzbQ2g8EeTtbJ%2BbZ8%3D&reserved=0 > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > arch/powerpc/platforms/powermac/backlight.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/arch/powerpc/platforms/powermac/backlight.c b/arch/powerpc/platforms/powermac/backlight.c > index 12bc01353bd3..79741370c40c 100644 > --- a/arch/powerpc/platforms/powermac/backlight.c > +++ b/arch/powerpc/platforms/powermac/backlight.c > @@ -57,18 +57,10 @@ struct backlight_device *pmac_backlight; > int pmac_has_backlight_type(const char *type) > { > struct device_node* bk_node = of_find_node_by_name(NULL, "backlight"); > + int i = of_property_match_string(bk_node, "backlight-control", type); > > - if (bk_node) { > - const char *prop = of_get_property(bk_node, > - "backlight-control", NULL); > - if (prop && strncmp(prop, type, strlen(type)) == 0) { > - of_node_put(bk_node); > - return 1; > - } > - of_node_put(bk_node); > - } > - > - return 0; > + of_node_put(bk_node); > + return i >= 0; Could have been: return !IS_ERR_VALUE(i); Never mind, Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > } > > static void pmac_backlight_key_worker(struct work_struct *work) > -- > 2.46.1 >
On Fri, Oct 11, 2024 at 06:25:45PM +0200, Christophe Leroy wrote: > > diff --git a/arch/powerpc/platforms/powermac/backlight.c b/arch/powerpc/platforms/powermac/backlight.c > > index 12bc01353bd3..79741370c40c 100644 > > --- a/arch/powerpc/platforms/powermac/backlight.c > > +++ b/arch/powerpc/platforms/powermac/backlight.c > > @@ -57,18 +57,10 @@ struct backlight_device *pmac_backlight; > > int pmac_has_backlight_type(const char *type) > > { > > struct device_node* bk_node = of_find_node_by_name(NULL, "backlight"); > > + int i = of_property_match_string(bk_node, "backlight-control", type); > > > > - if (bk_node) { > > - const char *prop = of_get_property(bk_node, > > - "backlight-control", NULL); > > - if (prop && strncmp(prop, type, strlen(type)) == 0) { > > - of_node_put(bk_node); > > - return 1; > > - } > > - of_node_put(bk_node); > > - } > > - > > - return 0; > > + of_node_put(bk_node); > > + return i >= 0; > > Could have been: > > return !IS_ERR_VALUE(i); > IS_ERR_VALUE() macro should only be used when you're dealing with memory addresses. What I mean is there places in mm/ where we pass addresses as unsigned long values instead of pointers. For example, get_unmapped_area() returns unsigned long. The IS_ERR_VALUE() macro is necessary for that. For regular error codes, we can just check for negatives. we don't have do anything fancy. Of course, you can find counter examples, like msm_iommu_attach_dev() or st_fdma_of_xlate(). <small joke>But in those cases, it's done to deliberately to ensure that the code will never accidentally get built on 64bit systems. </small joke> regards, dan carpenter
diff --git a/arch/powerpc/platforms/powermac/backlight.c b/arch/powerpc/platforms/powermac/backlight.c index 12bc01353bd3..d3666595a62e 100644 --- a/arch/powerpc/platforms/powermac/backlight.c +++ b/arch/powerpc/platforms/powermac/backlight.c @@ -61,11 +61,9 @@ int pmac_has_backlight_type(const char *type) if (bk_node) { const char *prop = of_get_property(bk_node, "backlight-control", NULL); - if (prop && strncmp(prop, type, strlen(type)) == 0) { - of_node_put(bk_node); - return 1; - } of_node_put(bk_node); + if (prop && strncmp(prop, type, strlen(type)) == 0) + return 1; } return 0;