| Message ID | 1460609956-26539-1-git-send-email-j-keerthy@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Keerthy <j-keerthy@ti.com> [160413 22:00]: > pcs_parse_bits_in_pinctrl_entry uses ffs which gives bit indices > ranging from 1 to MAX. This leads to a corner case where we try to request > the pin number = MAX and fails. > > bit_pos value is being calculted using ffs. pin_num_from_lsb uses > bit_pos value. pins array is populated with: > > pin + pin_num_from_lsb. > > The above is 1 more than usual bit indices as bit_pos uses ffs to compute > first set bit. Hence the last of the pins array is populated with the MAX > value and not MAX - 1 which causes error when we call pin_request. > > mask_pos is rightly calculated as ((pcs->fmask) << (bit_pos - 1)) > Consequently val_pos and submask are correct. > > Hence use __ffs which gives (ffs(x) - 1) as the first bit set. > > fixes: 4e7e8017a8 ("pinctrl: pinctrl-single: enhance to configure multiple pins of different modules") > Signed-off-by: Keerthy <j-keerthy@ti.com> > --- > > Changes in v2: > > * Changed pcs->fshift to use __ffs instead of ffs to be consistent. Thanks for updating it, looks good to me and still works here: Acked-by: Tony Lindgren <tony@atomide.com> > Boot tesed on da850-evm and checked the pinctrl sysfs nodes. > > drivers/pinctrl/pinctrl-single.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > index fb126d5..cf9bafa 100644 > --- a/drivers/pinctrl/pinctrl-single.c > +++ b/drivers/pinctrl/pinctrl-single.c > @@ -1280,9 +1280,9 @@ static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs, > > /* Parse pins in each row from LSB */ > while (mask) { > - bit_pos = ffs(mask); > + bit_pos = __ffs(mask); > pin_num_from_lsb = bit_pos / pcs->bits_per_pin; > - mask_pos = ((pcs->fmask) << (bit_pos - 1)); > + mask_pos = ((pcs->fmask) << bit_pos); > val_pos = val & mask_pos; > submask = mask & mask_pos; > > @@ -1852,7 +1852,7 @@ static int pcs_probe(struct platform_device *pdev) > ret = of_property_read_u32(np, "pinctrl-single,function-mask", > &pcs->fmask); > if (!ret) { > - pcs->fshift = ffs(pcs->fmask) - 1; > + pcs->fshift = __ffs(pcs->fmask); > pcs->fmax = pcs->fmask >> pcs->fshift; > } else { > /* If mask property doesn't exist, function mux is invalid. */ > -- > 1.9.1 >
On Thu, Apr 14, 2016 at 6:59 AM, Keerthy <j-keerthy@ti.com> wrote: > pcs_parse_bits_in_pinctrl_entry uses ffs which gives bit indices > ranging from 1 to MAX. This leads to a corner case where we try to request > the pin number = MAX and fails. > > bit_pos value is being calculted using ffs. pin_num_from_lsb uses > bit_pos value. pins array is populated with: > > pin + pin_num_from_lsb. > > The above is 1 more than usual bit indices as bit_pos uses ffs to compute > first set bit. Hence the last of the pins array is populated with the MAX > value and not MAX - 1 which causes error when we call pin_request. > > mask_pos is rightly calculated as ((pcs->fmask) << (bit_pos - 1)) > Consequently val_pos and submask are correct. > > Hence use __ffs which gives (ffs(x) - 1) as the first bit set. > > fixes: 4e7e8017a8 ("pinctrl: pinctrl-single: enhance to configure multiple pins of different modules") > Signed-off-by: Keerthy <j-keerthy@ti.com> > --- > > Changes in v2: > > * Changed pcs->fshift to use __ffs instead of ffs to be consistent. > > Boot tesed on da850-evm and checked the pinctrl sysfs nodes. Patch applied for fixes with Tony's ACK. Should it also be tagged for stable? Yours, Linus Walleij
* Linus Walleij <linus.walleij@linaro.org> [160415 02:29]: > On Thu, Apr 14, 2016 at 6:59 AM, Keerthy <j-keerthy@ti.com> wrote: > > > pcs_parse_bits_in_pinctrl_entry uses ffs which gives bit indices > > ranging from 1 to MAX. This leads to a corner case where we try to request > > the pin number = MAX and fails. > > > > bit_pos value is being calculted using ffs. pin_num_from_lsb uses > > bit_pos value. pins array is populated with: > > > > pin + pin_num_from_lsb. > > > > The above is 1 more than usual bit indices as bit_pos uses ffs to compute > > first set bit. Hence the last of the pins array is populated with the MAX > > value and not MAX - 1 which causes error when we call pin_request. > > > > mask_pos is rightly calculated as ((pcs->fmask) << (bit_pos - 1)) > > Consequently val_pos and submask are correct. > > > > Hence use __ffs which gives (ffs(x) - 1) as the first bit set. > > > > fixes: 4e7e8017a8 ("pinctrl: pinctrl-single: enhance to configure multiple pins of different modules") > > Signed-off-by: Keerthy <j-keerthy@ti.com> > > --- > > > > Changes in v2: > > > > * Changed pcs->fshift to use __ffs instead of ffs to be consistent. > > > > Boot tesed on da850-evm and checked the pinctrl sysfs nodes. > > Patch applied for fixes with Tony's ACK. > > Should it also be tagged for stable? Probably a good idea, I can see somebody pulling hair out because of this in various product trees. Regards, Tony
On Fri, Apr 15, 2016 at 5:22 PM, Tony Lindgren <tony@atomide.com> wrote: > * Linus Walleij <linus.walleij@linaro.org> [160415 02:29]: >> On Thu, Apr 14, 2016 at 6:59 AM, Keerthy <j-keerthy@ti.com> wrote: >> >> > pcs_parse_bits_in_pinctrl_entry uses ffs which gives bit indices >> > ranging from 1 to MAX. This leads to a corner case where we try to request >> > the pin number = MAX and fails. >> > >> > bit_pos value is being calculted using ffs. pin_num_from_lsb uses >> > bit_pos value. pins array is populated with: >> > >> > pin + pin_num_from_lsb. >> > >> > The above is 1 more than usual bit indices as bit_pos uses ffs to compute >> > first set bit. Hence the last of the pins array is populated with the MAX >> > value and not MAX - 1 which causes error when we call pin_request. >> > >> > mask_pos is rightly calculated as ((pcs->fmask) << (bit_pos - 1)) >> > Consequently val_pos and submask are correct. >> > >> > Hence use __ffs which gives (ffs(x) - 1) as the first bit set. >> > >> > fixes: 4e7e8017a8 ("pinctrl: pinctrl-single: enhance to configure multiple pins of different modules") >> > Signed-off-by: Keerthy <j-keerthy@ti.com> >> > --- >> > >> > Changes in v2: >> > >> > * Changed pcs->fshift to use __ffs instead of ffs to be consistent. >> > >> > Boot tesed on da850-evm and checked the pinctrl sysfs nodes. >> >> Patch applied for fixes with Tony's ACK. >> >> Should it also be tagged for stable? > > Probably a good idea, I can see somebody pulling hair out because > of this in various product trees. Ooops sorry I totally missed to add that :( Please ask Greg to take it as a selected stable patch. Yours, Linus Walleij
* Linus Walleij <linus.walleij@linaro.org> [160423 02:46]: > On Fri, Apr 15, 2016 at 5:22 PM, Tony Lindgren <tony@atomide.com> wrote: > > * Linus Walleij <linus.walleij@linaro.org> [160415 02:29]: > >> Patch applied for fixes with Tony's ACK. > >> > >> Should it also be tagged for stable? > > > > Probably a good idea, I can see somebody pulling hair out because > > of this in various product trees. > > Ooops sorry I totally missed to add that :( > > Please ask Greg to take it as a selected stable patch. Well it has the fixes tag, let's see if the stable people will find it automatically now :) If not, Keerthy can send email requesting adding it manually to stable trees. Tony
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index fb126d5..cf9bafa 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -1280,9 +1280,9 @@ static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs, /* Parse pins in each row from LSB */ while (mask) { - bit_pos = ffs(mask); + bit_pos = __ffs(mask); pin_num_from_lsb = bit_pos / pcs->bits_per_pin; - mask_pos = ((pcs->fmask) << (bit_pos - 1)); + mask_pos = ((pcs->fmask) << bit_pos); val_pos = val & mask_pos; submask = mask & mask_pos; @@ -1852,7 +1852,7 @@ static int pcs_probe(struct platform_device *pdev) ret = of_property_read_u32(np, "pinctrl-single,function-mask", &pcs->fmask); if (!ret) { - pcs->fshift = ffs(pcs->fmask) - 1; + pcs->fshift = __ffs(pcs->fmask); pcs->fmax = pcs->fmask >> pcs->fshift; } else { /* If mask property doesn't exist, function mux is invalid. */
pcs_parse_bits_in_pinctrl_entry uses ffs which gives bit indices ranging from 1 to MAX. This leads to a corner case where we try to request the pin number = MAX and fails. bit_pos value is being calculted using ffs. pin_num_from_lsb uses bit_pos value. pins array is populated with: pin + pin_num_from_lsb. The above is 1 more than usual bit indices as bit_pos uses ffs to compute first set bit. Hence the last of the pins array is populated with the MAX value and not MAX - 1 which causes error when we call pin_request. mask_pos is rightly calculated as ((pcs->fmask) << (bit_pos - 1)) Consequently val_pos and submask are correct. Hence use __ffs which gives (ffs(x) - 1) as the first bit set. fixes: 4e7e8017a8 ("pinctrl: pinctrl-single: enhance to configure multiple pins of different modules") Signed-off-by: Keerthy <j-keerthy@ti.com> --- Changes in v2: * Changed pcs->fshift to use __ffs instead of ffs to be consistent. Boot tesed on da850-evm and checked the pinctrl sysfs nodes. drivers/pinctrl/pinctrl-single.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)