LinuxLists.cc - [PATCH v1] gpio: gpiolib: clear the array

2023-04-23 11:06:24

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

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

Signed-off-by: Yan Wang <[email protected]>
---
drivers/gpio/gpiolib.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 04fb05df805b..cdaffcdd45b2 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4340,8 +4340,11 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
}

/* If there is no cache for fast bitmap processing path, continue */
- if (!array_info)
+ if (!array_info) {
+ /*clear descs->info*/
+ memset(array_info, 0, sizeof(struct gpio_array));
continue;
+ }

/* Unmark array members which don't belong to the 'fast' chip */
if (array_info->chip != gc) {
--
2.17.1

2023-04-23 13:17:01

by kernel test robot

[permalink] [raw]

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

2023-04-23 13:48:52

by kernel test robot

[permalink] [raw]

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

Hi Yan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on next-20230421]
[cannot apply to linus/master v6.3-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Yan-Wang/gpio-gpiolib-clear-the-array_info-s-memory-space/20230423-190500
base: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link: https://lore.kernel.org/r/KL1PR01MB54489B7A3D9D02D242B4BDA1E6669%40KL1PR01MB5448.apcprd01.prod.exchangelabs.com
patch subject: [PATCH v1] gpio: gpiolib: clear the array_info's memory space
config: microblaze-buildonly-randconfig-r006-20230423 (https://download.01.org/0day-ci/archive/20230423/[email protected]/config)
compiler: microblaze-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/061c9f6937fab64a9c1d051252fcd3236a35381f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yan-Wang/gpio-gpiolib-clear-the-array_info-s-memory-space/20230423-190500
git checkout 061c9f6937fab64a9c1d051252fcd3236a35381f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=microblaze olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/gpio/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

drivers/gpio/gpiolib.c: In function 'gpiod_get_array':
>> drivers/gpio/gpiolib.c:4345:25: warning: argument 1 null where non-null expected [-Wnonnull]
4345 | memset(array_info, 0, sizeof(struct gpio_array));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/string.h:20,
from include/linux/bitmap.h:11,
from include/linux/cpumask.h:12,
from include/linux/smp.h:13,
from include/linux/lockdep.h:14,
from include/linux/mutex.h:17,
from include/linux/kernfs.h:11,
from include/linux/sysfs.h:16,
from include/linux/kobject.h:20,
from include/linux/of.h:17,
from include/linux/irqdomain.h:36,
from include/linux/acpi.h:13,
from drivers/gpio/gpiolib.c:3:
arch/microblaze/include/asm/string.h:16:14: note: in a call to function 'memset' declared 'nonnull'
16 | extern void *memset(void *, int, __kernel_size_t);
| ^~~~~~

vim +4345 drivers/gpio/gpiolib.c

4263
4264 /**
4265 * gpiod_get_array - obtain multiple GPIOs from a multi-index GPIO function
4266 * @dev: GPIO consumer, can be NULL for system-global GPIOs
4267 * @con_id: function within the GPIO consumer
4268 * @flags: optional GPIO initialization flags
4269 *
4270 * This function acquires all the GPIOs defined under a given function.
4271 *
4272 * Return a struct gpio_descs containing an array of descriptors, -ENOENT if
4273 * no GPIO has been assigned to the requested function, or another IS_ERR()
4274 * code if an error occurred while trying to acquire the GPIOs.
4275 */
4276 struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
4277 const char *con_id,
4278 enum gpiod_flags flags)
4279 {
4280 struct gpio_desc *desc;
4281 struct gpio_descs *descs;
4282 struct gpio_array *array_info = NULL;
4283 struct gpio_chip *gc;
4284 int count, bitmap_size;
4285 size_t descs_size;
4286
4287 count = gpiod_count(dev, con_id);
4288 if (count < 0)
4289 return ERR_PTR(count);
4290
4291 descs_size = struct_size(descs, desc, count);
4292 descs = kzalloc(descs_size, GFP_KERNEL);
4293 if (!descs)
4294 return ERR_PTR(-ENOMEM);
4295
4296 for (descs->ndescs = 0; descs->ndescs < count; descs->ndescs++) {
4297 desc = gpiod_get_index(dev, con_id, descs->ndescs, flags);
4298 if (IS_ERR(desc)) {
4299 gpiod_put_array(descs);
4300 return ERR_CAST(desc);
4301 }
4302
4303 descs->desc[descs->ndescs] = desc;
4304
4305 gc = gpiod_to_chip(desc);
4306 /*
4307 * If pin hardware number of array member 0 is also 0, select
4308 * its chip as a candidate for fast bitmap processing path.
4309 */
4310 if (descs->ndescs == 0 && gpio_chip_hwgpio(desc) == 0) {
4311 struct gpio_descs *array;
4312
4313 bitmap_size = BITS_TO_LONGS(gc->ngpio > count ?
4314 gc->ngpio : count);
4315
4316 array = krealloc(descs, descs_size +
4317 struct_size(array_info, invert_mask, 3 * bitmap_size),
4318 GFP_KERNEL | __GFP_ZERO);
4319 if (!array) {
4320 gpiod_put_array(descs);
4321 return ERR_PTR(-ENOMEM);
4322 }
4323
4324 descs = array;
4325
4326 array_info = (void *)descs + descs_size;
4327 array_info->get_mask = array_info->invert_mask +
4328 bitmap_size;
4329 array_info->set_mask = array_info->get_mask +
4330 bitmap_size;
4331
4332 array_info->desc = descs->desc;
4333 array_info->size = count;
4334 array_info->chip = gc;
4335 bitmap_set(array_info->get_mask, descs->ndescs,
4336 count - descs->ndescs);
4337 bitmap_set(array_info->set_mask, descs->ndescs,
4338 count - descs->ndescs);
4339 descs->info = array_info;
4340 }
4341
4342 /* If there is no cache for fast bitmap processing path, continue */
4343 if (!array_info) {
4344 /*clear descs->info*/
> 4345 memset(array_info, 0, sizeof(struct gpio_array));
4346 continue;
4347 }
4348
4349 /* Unmark array members which don't belong to the 'fast' chip */
4350 if (array_info->chip != gc) {
4351 __clear_bit(descs->ndescs, array_info->get_mask);
4352 __clear_bit(descs->ndescs, array_info->set_mask);
4353 }
4354 /*
4355 * Detect array members which belong to the 'fast' chip
4356 * but their pins are not in hardware order.
4357 */
4358 else if (gpio_chip_hwgpio(desc) != descs->ndescs) {
4359 /*
4360 * Don't use fast path if all array members processed so
4361 * far belong to the same chip as this one but its pin
4362 * hardware number is different from its array index.
4363 */
4364 if (bitmap_full(array_info->get_mask, descs->ndescs)) {
4365 array_info = NULL;
4366 } else {
4367 __clear_bit(descs->ndescs,
4368 array_info->get_mask);
4369 __clear_bit(descs->ndescs,
4370 array_info->set_mask);
4371 }
4372 } else {
4373 /* Exclude open drain or open source from fast output */
4374 if (gpiochip_line_is_open_drain(gc, descs->ndescs) ||
4375 gpiochip_line_is_open_source(gc, descs->ndescs))
4376 __clear_bit(descs->ndescs,
4377 array_info->set_mask);
4378 /* Identify 'fast' pins which require invertion */
4379 if (gpiod_is_active_low(desc))
4380 __set_bit(descs->ndescs,
4381 array_info->invert_mask);
4382 }
4383 }
4384 if (array_info)
4385 dev_dbg(dev,
4386 "GPIO array info: chip=%s, size=%d, get_mask=%lx, set_mask=%lx, invert_mask=%lx\n",
4387 array_info->chip->label, array_info->size,
4388 *array_info->get_mask, *array_info->set_mask,
4389 *array_info->invert_mask);
4390 return descs;
4391 }
4392 EXPORT_SYMBOL_GPL(gpiod_get_array);
4393

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-04-23 14:04:34

by Yan Wang

[permalink] [raw]

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

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 <[email protected]>
Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Signed-off-by: Yan Wang <[email protected]>
---
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

2023-04-26 14:54:44

by Andy Shevchenko

[permalink] [raw]

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

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

2023-04-27 06:17:35

by Yan Wang

[permalink] [raw]

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

On 4/26/2023 10:42 PM, [email protected] 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;
> ...
>
>> }

2023-05-04 09:41:39

by Yan Wang

[permalink] [raw]

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

On 4/26/2023 10:42 PM, [email protected] 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;
> ...
>
>> }

2023-05-04 11:42:19

by Andy Shevchenko

[permalink] [raw]

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

On Thu, May 4, 2023 at 12:38 PM Yan Wang <[email protected]> wrote:
> On 4/26/2023 10:42 PM, [email protected] 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

2023-05-04 14:40:04

by Yan Wang

[permalink] [raw]

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

> On May 4, 2023, at 19:36, Andy Shevchenko <[email protected]> wrote:
>
> On Thu, May 4, 2023 at 12:38 PM Yan Wang <[email protected]> wrote:
>> On 4/26/2023 10:42 PM, [email protected] 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

2023-05-12 08:02:59

by Yan Wang

[permalink] [raw]

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

Polite ping

On 5/4/2023 10:15 PM, Yan Wang wrote:
>
>> On May 4, 2023, at 19:36, Andy Shevchenko <[email protected]> wrote:
>>
>> On Thu, May 4, 2023 at 12:38 PM Yan Wang <[email protected]> wrote:
>>> On 4/26/2023 10:42 PM, [email protected] 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

2023-05-15 07:41:55

by Linus Walleij

[permalink] [raw]

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

> @@ -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