[RFC,7/7] i2c: core: hand over reserved devices when requesting ancillary addresses
diff mbox series

Message ID 20200220172403.26062-8-wsa+renesas@sang-engineering.com
State RFC
Headers show
Series
  • i2c: of: reserve unknown and ancillary addresses
Related show

Commit Message

Wolfram Sang Feb. 20, 2020, 5:24 p.m. UTC
With i2c_new_ancillary_address, we can check if the intended driver is
requesting a reserved address. Update the function to do these checks.
If the check passes, the "reserved" device will become a regular "dummy"
device.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Geert Uytterhoeven Feb. 21, 2020, 10:13 a.m. UTC | #1
Hi Wolfram,

On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> With i2c_new_ancillary_address, we can check if the intended driver is
> requesting a reserved address. Update the function to do these checks.
> If the check passes, the "reserved" device will become a regular "dummy"
> device.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -975,6 +975,8 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
>                                                 u16 default_addr)
>  {
>         struct device_node *np = client->dev.of_node;
> +       struct device *reserved_dev, *adapter_dev = &client->adapter->dev;
> +       struct i2c_client *reserved_client;
>         u32 addr = default_addr;
>         int i;
>
> @@ -984,7 +986,21 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
>                         of_property_read_u32_index(np, "reg", i, &addr);
>         }
>
> -       dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
> +       dev_info(adapter_dev, "Address for %s : 0x%x\n", name, addr);
> +
> +       /* No need to scan muxes, siblings must sit on the same adapter */
> +       reserved_dev = device_find_child(adapter_dev, &addr, __i2c_check_addr_busy);
> +       reserved_client = i2c_verify_client(reserved_dev);
> +
> +       if (reserved_client) {
> +               if (reserved_client->dev.of_node != np ||
> +                   strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0)
> +                       return ERR_PTR(-EBUSY);

Missing put_device(reserved_dev).

> +
> +               strlcpy(reserved_client->name, I2C_DUMMY_DRV_NAME, sizeof(client->name));
> +               return reserved_client;
> +       }

else put_device(reserved_dev)

(perhaps i2c_verify_client() checking dev was not such a great idea, as
 callers need to act on dev && !verified anyway?)

> +
>         return i2c_new_dummy_device(client->adapter, addr);
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_ancillary_device);

Gr{oetje,eeting}s,

                        Geert
Luca Ceresoli Feb. 28, 2020, 12:11 p.m. UTC | #2
Hi,

On 21/02/20 11:13, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>> With i2c_new_ancillary_address, we can check if the intended driver is
>> requesting a reserved address. Update the function to do these checks.
>> If the check passes, the "reserved" device will become a regular "dummy"
>> device.
>>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -975,6 +975,8 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
>>                                                 u16 default_addr)
>>  {
>>         struct device_node *np = client->dev.of_node;
>> +       struct device *reserved_dev, *adapter_dev = &client->adapter->dev;
>> +       struct i2c_client *reserved_client;
>>         u32 addr = default_addr;
>>         int i;
>>
>> @@ -984,7 +986,21 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
>>                         of_property_read_u32_index(np, "reg", i, &addr);
>>         }
>>
>> -       dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
>> +       dev_info(adapter_dev, "Address for %s : 0x%x\n", name, addr);
>> +
>> +       /* No need to scan muxes, siblings must sit on the same adapter */
>> +       reserved_dev = device_find_child(adapter_dev, &addr, __i2c_check_addr_busy);
>> +       reserved_client = i2c_verify_client(reserved_dev);
>> +
>> +       if (reserved_client) {
>> +               if (reserved_client->dev.of_node != np ||
>> +                   strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0)
>> +                       return ERR_PTR(-EBUSY);
> 
> Missing put_device(reserved_dev).
> 
>> +
>> +               strlcpy(reserved_client->name, I2C_DUMMY_DRV_NAME, sizeof(client->name));

Any strong reason for not giving the device a more informative name?
Reading "dummy" in several /sys/bus/i2c/devices/?-????/name files is not
helping. Using the 'name' string that is passed to
i2c_new_ancillary_device() would be way better, perhaps prefixed by
dev->name. But this opens the question of why not doing it in
i2c_new_dummy_device() as well, which currently receives no "name"
parameter.

Of course this is not strictly related to this patch and can be done in
a later step.

About the patch itself, except for the issues pointed out by Geert the
approach looks generally good to me.
Wolfram Sang March 12, 2020, 11:21 a.m. UTC | #3
> (perhaps i2c_verify_client() checking dev was not such a great idea, as
>  callers need to act on dev && !verified anyway?)

Can be argued. I will have a second thought about it.
Wolfram Sang March 12, 2020, 11:30 a.m. UTC | #4
> >> +               strlcpy(reserved_client->name, I2C_DUMMY_DRV_NAME, sizeof(client->name));
> 
> Any strong reason for not giving the device a more informative name?

Yes, sadly...

> Reading "dummy" in several /sys/bus/i2c/devices/?-????/name files is not
> helping. Using the 'name' string that is passed to
> i2c_new_ancillary_device() would be way better, perhaps prefixed by
> dev->name. But this opens the question of why not doing it in

... I never liked the plain "dummy" name as well. However, because
'name' is what we need to bind to a driver we can't have a more
descriptive or run-time generated name at that place.

> i2c_new_dummy_device() as well, which currently receives no "name"
> parameter.

I thought about it but discarded the idea because then you still have
no connection to the driver which created the dummy device. My
favourite idea so far is to advertise i2c_new_ancillary_device() instead
of i2c_new_dummy_device(), because there we already have access to the
client structure. With that, we could add another link in sysfs to the
main address and vice-versa.

> Of course this is not strictly related to this patch and can be done in
> a later step.

Exactly.
Wolfram Sang March 13, 2020, 12:42 p.m. UTC | #5
> > @@ -984,7 +986,21 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
> >                         of_property_read_u32_index(np, "reg", i, &addr);
> >         }
> >
> > -       dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
> > +       dev_info(adapter_dev, "Address for %s : 0x%x\n", name, addr);
> > +
> > +       /* No need to scan muxes, siblings must sit on the same adapter */
> > +       reserved_dev = device_find_child(adapter_dev, &addr, __i2c_check_addr_busy);
> > +       reserved_client = i2c_verify_client(reserved_dev);
> > +
> > +       if (reserved_client) {
> > +               if (reserved_client->dev.of_node != np ||
> > +                   strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0)
> > +                       return ERR_PTR(-EBUSY);
> 
> Missing put_device(reserved_dev).

Actually, I think the code could even be like this:

	struct i2c_client *reserved_client = NULL;

	...

	reserved_dev = device_find_child(adapter_dev, &addr, __i2c_check_addr_busy);
	if (reserved_dev) {
		reserved_np = reserved_dev->of_node;
		reserved_client = i2c_verify_client(reserved_dev);
		put_device(reserved_dev);
	}

	if (reserved_client) {
		if (reserved_np != np ||
		    strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0)
			return ERR_PTR(-EBUSY);

		strlcpy(reserved_client->name, I2C_DUMMY_DRV_NAME, sizeof(client->name));
		return reserved_client;
	}

	return i2c_new_dummy_device(client->adapter, addr);

We put the device early - as soon we don't access the struct anymore. I
think we don't need the refcnt any further because what we are doing
here is to hand over the initial refcnt from the core to the requesting
driver. We turn the device from "reserved" (internally managed) to
"dummy" (managed by the driver). So, I think the code is okay regarding
the struct device. I will have a second look when it comes to
concurrency problems regarding the struct i2c_client, though.

> (perhaps i2c_verify_client() checking dev was not such a great idea, as
>  callers need to act on dev && !verified anyway?)

Yeah, since I refactored the ACPI code as well, patch 1 from this series
can probably go.

Thanks again for your review, Geert!

Patch
diff mbox series

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 4000a4384306..ba325f8107a3 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -975,6 +975,8 @@  struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
 						u16 default_addr)
 {
 	struct device_node *np = client->dev.of_node;
+	struct device *reserved_dev, *adapter_dev = &client->adapter->dev;
+	struct i2c_client *reserved_client;
 	u32 addr = default_addr;
 	int i;
 
@@ -984,7 +986,21 @@  struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
 			of_property_read_u32_index(np, "reg", i, &addr);
 	}
 
-	dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
+	dev_info(adapter_dev, "Address for %s : 0x%x\n", name, addr);
+
+	/* No need to scan muxes, siblings must sit on the same adapter */
+	reserved_dev = device_find_child(adapter_dev, &addr, __i2c_check_addr_busy);
+	reserved_client = i2c_verify_client(reserved_dev);
+
+	if (reserved_client) {
+		if (reserved_client->dev.of_node != np ||
+		    strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0)
+			return ERR_PTR(-EBUSY);
+
+		strlcpy(reserved_client->name, I2C_DUMMY_DRV_NAME, sizeof(client->name));
+		return reserved_client;
+	}
+
 	return i2c_new_dummy_device(client->adapter, addr);
 }
 EXPORT_SYMBOL_GPL(i2c_new_ancillary_device);