[v2] gpio: gpiolib: clear the array_info's memory space

[PATCH v2] gpio: gpiolib: clear the array_info's memory space

Posted by Yan Wang 2 years, 7 months ago

if hardware number different to array index,it needs to clear to points
memory space if the array_info have been assigned a value.

Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/oe-kbuild-all/202304232146.7M89pwCz-lkp@intel.com/
Signed-off-by: Yan Wang <rk.code@outlook.com>
---
v1->v2: fixed building warning
---
 drivers/gpio/gpiolib.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 04fb05df805b..8b2a8db44b54 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4359,6 +4359,8 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 			 * hardware number is different from its array index.
 			 */
 			if (bitmap_full(array_info->get_mask, descs->ndescs)) {
+				/*clear descs->info*/
+				memset(array_info, 0, sizeof(struct gpio_array));
 				array_info = NULL;
 			} else {
 				__clear_bit(descs->ndescs,
-- 
2.17.1

Re: [PATCH v2] gpio: gpiolib: clear the array_info's memory space

Posted by Linus Walleij 2 years, 7 months ago

> @@ -4359,6 +4359,8 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,

>                          * hardware number is different from its array index.
>                          */
>                         if (bitmap_full(array_info->get_mask, descs->ndescs)) {
> +                               /*clear descs->info*/
> +                               memset(array_info, 0, sizeof(struct gpio_array));
>                                 array_info = NULL;

This is not the right solution.

The array_info points beyond descs and descs have be krealloc:ed
to fit the array info.

The right solution is not to fill that memory with zeroes, but to krealloc
back to the size that descs had before we did this resizing to begin
with.

Possibly the condition should be detected *before* we start to krealloc()
so we can avoid all the krealloc():ing.

If the actual issue cannot be fixed I think it is no better or worse to just
leave the code as it is, we are just zeroing some unused memory.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: gpiolib: clear the array_info's memory space

Posted by andy.shevchenko@gmail.com 2 years, 7 months ago

Sun, Apr 23, 2023 at 09:59:43PM +0800, Yan Wang kirjoitti:
> if hardware number different to array index,it needs to clear to points
> memory space if the array_info have been assigned a value.

Can you explain a bit more what's going on there?

...

>  			if (bitmap_full(array_info->get_mask, descs->ndescs)) {
> +				/*clear descs->info*/
> +				memset(array_info, 0, sizeof(struct gpio_array));
>  				array_info = NULL;

...

>  			}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: gpiolib: clear the array_info's memory space

Posted by Yan Wang 2 years, 7 months ago

On 4/26/2023 10:42 PM, andy.shevchenko@gmail.com wrote:
> Sun, Apr 23, 2023 at 09:59:43PM +0800, Yan Wang kirjoitti:
>> if hardware number different to array index,it needs to clear to points
>> memory space if the array_info have been assigned a value.
> Can you explain a bit more what's going on there?
>
> ...
Hi Andy,

I use gpiod_get_array() to get a gpio array form the node of DTS.

the node is as follows:
...
gpios = <&gpio1 0 0>, <&gpio1 10 0>;
...

First scan pin-0 of gpio1,its index and hardware number are 0,

if (descs->ndescs == 0 && gpio_chip_hwgpio(desc) == 0) {
     ...
     descs->info = array_info.
}

Then scan pin-10 , its index is 1 ,but hardware number is 10 .

if (gpio_chip_hwgpio(desc) != descs->ndescs) {
     array_info = NULL;
}
just set array_info = NULL, Should the array_info point to memory be 
cleared ?

if not cleared ,I use the gpiod_set_array_value_cansleep() to pull up or 
down pin-10 is invalid.

I found that the set_mask and get_mask vlaues of descs->info are seted 
0x03 in gpiod_get_array(),
I think 0x401 is their correct value.

Thank you for review.
>> if (bitmap_full(array_info->get_mask, descs->ndescs)) {
>> + /*clear descs->info*/
>> + memset(array_info, 0, sizeof(struct gpio_array));
>> array_info = NULL;
> ...
>
>> }

Re: [PATCH v2] gpio: gpiolib: clear the array_info's memory space

Posted by Andy Shevchenko 2 years, 7 months ago

On Thu, May 4, 2023 at 12:38 PM Yan Wang <rk.code@outlook.com> wrote:
> On 4/26/2023 10:42 PM, andy.shevchenko@gmail.com wrote:
> > Sun, Apr 23, 2023 at 09:59:43PM +0800, Yan Wang kirjoitti:
> >> if hardware number different to array index,it needs to clear to points
> >> memory space if the array_info have been assigned a value.
> > Can you explain a bit more what's going on there?

...

> I use gpiod_get_array() to get a gpio array form the node of DTS.
>
> the node is as follows:
> ...
> gpios = <&gpio1 0 0>, <&gpio1 10 0>;
> ...
>
> First scan pin-0 of gpio1,its index and hardware number are 0,
>
> if (descs->ndescs == 0 && gpio_chip_hwgpio(desc) == 0) {
>      ...
>      descs->info = array_info.
> }
>
> Then scan pin-10 , its index is 1 ,but hardware number is 10 .
>
> if (gpio_chip_hwgpio(desc) != descs->ndescs) {
>      array_info = NULL;
> }
> just set array_info = NULL, Should the array_info point to memory be
> cleared ?

This is a good question. The entire algorithm is a bit difficult to
understand from the first glance. I need some time to check it myself.

> if not cleared ,I use the gpiod_set_array_value_cansleep() to pull up or
> down pin-10 is invalid.

I'm not sure I follow. The array operations are against the given
array of the descriptors. If you ask to have that operation done, the
all descriptors in the array should be considered.

> I found that the set_mask and get_mask vlaues of descs->info are seted
> 0x03 in gpiod_get_array(),

Yes, this mask is for the argument. The 0x03 is the correct one.

> I think 0x401 is their correct value.

No. You have an array of two elements, and not 11.

> >> if (bitmap_full(array_info->get_mask, descs->ndescs)) {
> >> + /*clear descs->info*/
> >> + memset(array_info, 0, sizeof(struct gpio_array));
> >> array_info = NULL;
> >> }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: gpiolib: clear the array_info's memory space

Posted by Yan Wang 2 years, 7 months ago


> On May 4, 2023, at 19:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> On Thu, May 4, 2023 at 12:38 PM Yan Wang <rk.code@outlook.com> wrote:
>> On 4/26/2023 10:42 PM, andy.shevchenko@gmail.com wrote:
>>> Sun, Apr 23, 2023 at 09:59:43PM +0800, Yan Wang kirjoitti:
>>>> if hardware number different to array index,it needs to clear to points
>>>> memory space if the array_info have been assigned a value.
>>> Can you explain a bit more what's going on there?
> 
> ...
> 
>> I use gpiod_get_array() to get a gpio array form the node of DTS.
>> 
>> the node is as follows:
>> ...
>> gpios = <&gpio1 0 0>, <&gpio1 10 0>;
>> ...
>> 
>> First scan pin-0 of gpio1,its index and hardware number are 0,
>> 
>> if (descs->ndescs == 0 && gpio_chip_hwgpio(desc) == 0) {
>> ...
>> descs->info = array_info.
>> }
>> 
>> Then scan pin-10 , its index is 1 ,but hardware number is 10 .
>> 
>> if (gpio_chip_hwgpio(desc) != descs->ndescs) {
>> array_info = NULL;
>> }
>> just set array_info = NULL, Should the array_info point to memory be
>> cleared ?
> 
> This is a good question. The entire algorithm is a bit difficult to
> understand from the first glance. I need some time to check it myself.
Looking forward to your test results.
> 
>> if not cleared ,I use the gpiod_set_array_value_cansleep() to pull up or
>> down pin-10 is invalid.
> 
> I'm not sure I follow. The array operations are against the given
> array of the descriptors. If you ask to have that operation done, the
> all descriptors in the array should be considered.
> 
>> I found that the set_mask and get_mask vlaues of descs->info are seted
>> 0x03 in gpiod_get_array(),
> 
> Yes, this mask is for the argument. The 0x03 is the correct one.
> 
>> I think 0x401 is their correct value.
> 
> No. You have an array of two elements, and not 11.

Due to hardware number are 10 and 0 , so this mask is 0x401（bit10 and bit0 are 1）.

> 
>>>> if (bitmap_full(array_info->get_mask, descs->ndescs)) {
>>>> + /*clear descs->info*/
>>>> + memset(array_info, 0, sizeof(struct gpio_array));
>>>> array_info = NULL;
>>>> }
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2] gpio: gpiolib: clear the array_info's memory space

Posted by Yan Wang 2 years, 7 months ago

Polite ping

On 5/4/2023 10:15 PM, Yan Wang wrote:
>
>> On May 4, 2023, at 19:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>
>> On Thu, May 4, 2023 at 12:38 PM Yan Wang <rk.code@outlook.com> wrote:
>>> On 4/26/2023 10:42 PM, andy.shevchenko@gmail.com wrote:
>>>> Sun, Apr 23, 2023 at 09:59:43PM +0800, Yan Wang kirjoitti:
>>>>> if hardware number different to array index,it needs to clear to points
>>>>> memory space if the array_info have been assigned a value.
>>>> Can you explain a bit more what's going on there?
>> ...
>>
>>> I use gpiod_get_array() to get a gpio array form the node of DTS.
>>>
>>> the node is as follows:
>>> ...
>>> gpios = <&gpio1 0 0>, <&gpio1 10 0>;
>>> ...
>>>
>>> First scan pin-0 of gpio1,its index and hardware number are 0,
>>>
>>> if (descs->ndescs == 0 && gpio_chip_hwgpio(desc) == 0) {
>>> ...
>>> descs->info = array_info.
>>> }
>>>
>>> Then scan pin-10 , its index is 1 ,but hardware number is 10 .
>>>
>>> if (gpio_chip_hwgpio(desc) != descs->ndescs) {
>>> array_info = NULL;
>>> }
>>> just set array_info = NULL, Should the array_info point to memory be
>>> cleared ?
>> This is a good question. The entire algorithm is a bit difficult to
>> understand from the first glance. I need some time to check it myself.
> Looking forward to your test results.
>>> if not cleared ,I use the gpiod_set_array_value_cansleep() to pull up or
>>> down pin-10 is invalid.
>> I'm not sure I follow. The array operations are against the given
>> array of the descriptors. If you ask to have that operation done, the
>> all descriptors in the array should be considered.
>>
>>> I found that the set_mask and get_mask vlaues of descs->info are seted
>>> 0x03 in gpiod_get_array(),
>> Yes, this mask is for the argument. The 0x03 is the correct one.
>>
>>> I think 0x401 is their correct value.
>> No. You have an array of two elements, and not 11.
> Due to hardware number are 10 and 0 , so this mask is 0x401（bit10 and bit0 are 1）.
>
>>>>> if (bitmap_full(array_info->get_mask, descs->ndescs)) {
>>>>> + /*clear descs->info*/
>>>>> + memset(array_info, 0, sizeof(struct gpio_array));
>>>>> array_info = NULL;
>>>>> }
>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko

Re: [PATCH v2] gpio: gpiolib: clear the array_info's memory space

Posted by Yan Wang 2 years, 7 months ago


On 4/26/2023 10:42 PM, andy.shevchenko@gmail.com wrote:
> Sun, Apr 23, 2023 at 09:59:43PM +0800, Yan Wang kirjoitti:
>> if hardware number different to array index,it needs to clear to points
>> memory space if the array_info have been assigned a value.
> Can you explain a bit more what's going on there?
>
> ...
Hi Andy,

I use gpiod_get_array() to get a gpio array form the node of DTS.

the node is as follows:
...
gpios = <&gpio1 0 0>, <&gpio1 10 0>;
...
first scan 0 pin of gpio1, its index and hardware number are 0,
the (descs->ndescs == 0 && gpio_chip_hwgpio(desc) == 0) is true and 
descs->info = array_info. then scan 10 pin , its index is 1 ,but 
hardware number is 10 , the  (gpio_chip_hwgpio(desc) != descs->ndescs) 
is true. array_info = NULL, Just make array_info point to NULL, Did't 
clean descs->info memory or point it to NULL. Should the array_info 
point to memory be cleared ? if not cleared ,I use 
gpiod_set_array_value_cansleep() setting pin 10 of gpio1 is invalid. I 
found that the set_mask and get_mask vlaues of descs->info are seted 
0x03 in gpiod_get_array(), I think 0x401 is their correct value. Thank 
you review
>>   			if (bitmap_full(array_info->get_mask, descs->ndescs)) {
>> +				/*clear descs->info*/
>> +				memset(array_info, 0, sizeof(struct gpio_array));
>>   				array_info = NULL;
> ...
>
>>   			}