| Message ID | 20200220172403.26062-8-wsa+renesas@sang-engineering.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | i2c: of: reserve unknown and ancillary addresses | expand |
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
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.
> (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.
> >> + 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.
> > @@ -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!
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);
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(-)