The chip_name configs attribute always displays the device name of the
first GPIO bank because the logic of the relevant function is simply
wrong.

Fix it by correctly comparing the bank's swnode against the GPIO
device's children.

Fixes: cb8c474e79be ("gpio: sim: new testing module")
Cc: stable@vger.kernel.org
Reported-by: Kent Gibson <warthog618@gmail.com>
Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-sim.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 98109839102f..a370d3aec6d9 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -991,7 +991,7 @@ static struct configfs_attribute *gpio_sim_device_config_attrs[] = {
 };
 
 struct gpio_sim_chip_name_ctx {
-	struct gpio_sim_device *dev;
+	struct fwnode_handle *swnode;
 	char *page;
 };
 
@@ -999,7 +999,6 @@ static int gpio_sim_emit_chip_name(struct device *dev, void *data)
 {
 	struct gpio_sim_chip_name_ctx *ctx = data;
 	struct fwnode_handle *swnode;
-	struct gpio_sim_bank *bank;
 
 	/* This would be the sysfs device exported in /sys/class/gpio. */
 	if (dev->class)
@@ -1007,12 +1006,10 @@ static int gpio_sim_emit_chip_name(struct device *dev, void *data)
 
 	swnode = dev_fwnode(dev);
 
-	list_for_each_entry(bank, &ctx->dev->bank_list, siblings) {
-		if (bank->swnode == swnode)
-			return sprintf(ctx->page, "%s\n", dev_name(dev));
-	}
+	if (swnode == ctx->swnode)
+		return sprintf(ctx->page, "%s\n", dev_name(dev));
 
-	return -ENODATA;
+	return 0;
 }
 
 static ssize_t gpio_sim_bank_config_chip_name_show(struct config_item *item,
@@ -1020,7 +1017,7 @@ static ssize_t gpio_sim_bank_config_chip_name_show(struct config_item *item,
 {
 	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
 	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
-	struct gpio_sim_chip_name_ctx ctx = { dev, page };
+	struct gpio_sim_chip_name_ctx ctx = { bank->swnode, page };
 	int ret;
 
 	mutex_lock(&dev->lock);
-- 
2.34.1

On Mon, Jul 11, 2022 at 07:34:18PM +0200, Bartosz Golaszewski wrote:
> The chip_name configs attribute always displays the device name of the
> first GPIO bank because the logic of the relevant function is simply
> wrong.
> 
> Fix it by correctly comparing the bank's swnode against the GPIO
> device's children.
> 

That works for me, so thanks for that.

Not totally convinced by Andy's suggestion to rename swnode to fwnode.
Variables should be named for what they represent, not their type, and
you use swnode extensively elsewhere in the module, so either change it
everywhere or not at all, and such a sweeping change is beyond the scope
this patch.

Though it may make his other suggestion to use device_match_fwnode()
read a little better.  No issue with that suggestion.

Cheers,
Kent.

On Tue, Jul 12, 2022 at 4:40 AM Kent Gibson <warthog618@gmail.com> wrote:
> On Mon, Jul 11, 2022 at 07:34:18PM +0200, Bartosz Golaszewski wrote:

...

> Not totally convinced by Andy's suggestion to rename swnode to fwnode.
> Variables should be named for what they represent, not their type, and
> you use swnode extensively elsewhere in the module, so either change it
> everywhere or not at all, and such a sweeping change is beyond the scope
> this patch.

Ah, I agree that consistency has higher priority here.

-- 
With Best Regards,
Andy Shevchenko

On Tue, Jul 12, 2022 at 4:37 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Jul 11, 2022 at 07:34:18PM +0200, Bartosz Golaszewski wrote:
> > The chip_name configs attribute always displays the device name of the
> > first GPIO bank because the logic of the relevant function is simply
> > wrong.
> >
> > Fix it by correctly comparing the bank's swnode against the GPIO
> > device's children.
> >
>
> That works for me, so thanks for that.
>
> Not totally convinced by Andy's suggestion to rename swnode to fwnode.
> Variables should be named for what they represent, not their type, and
> you use swnode extensively elsewhere in the module, so either change it
> everywhere or not at all, and such a sweeping change is beyond the scope
> this patch.
>
> Though it may make his other suggestion to use device_match_fwnode()
> read a little better.  No issue with that suggestion.
>
> Cheers,
> Kent.

I agree on device_match_fwnode() and disagree on the swnode rename. v2 sent out.

Bart

On Mon, Jul 11, 2022 at 7:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> The chip_name configs attribute always displays the device name of the
> first GPIO bank because the logic of the relevant function is simply
> wrong.
>
> Fix it by correctly comparing the bank's swnode against the GPIO
> device's children.
>
> Fixes: cb8c474e79be ("gpio: sim: new testing module")
> Cc: stable@vger.kernel.org
> Reported-by: Kent Gibson <warthog618@gmail.com>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

...

>  struct gpio_sim_chip_name_ctx {
> -       struct gpio_sim_device *dev;
> +       struct fwnode_handle *swnode;

I would call it fwnode even if we know the backend provider.

>         char *page;
>  };

...

>         struct fwnode_handle *swnode;

Do you still need this? See below.

...

>         swnode = dev_fwnode(dev);
>
> +       if (swnode == ctx->swnode)
> +               return sprintf(ctx->page, "%s\n", dev_name(dev));

So, now it can be

if (device_match_fwnode(dev, ctx->fwnode))
  return sprintf(...);

-- 
With Best Regards,
Andy Shevchenko