Re: [PATCH v3 05/13] i2c: acpi: Return error pointers from i2c_acpi_new_device()

[Hans de Goede]

Hi,

On 27-11-18 17:14, Hans de Goede wrote:
> Hi,
>
> On 27-11-18 16:37, Andy Shevchenko wrote:
> > The caller would like to know the reason why the i2c_acpi_new_device() fails.
> > For example, if adapter is not available, it might be in the future and we
> > would like to re-probe the clients again. But at the same time we would like to
> > bail out if the error seems unrecoverable, such as invalid argument supplied.
> > To achieve this, return error pointer in some cases.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
> > Â drivers/i2c/i2c-core-acpi.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ | 21 ++++++++++++++------
> > Â drivers/platform/x86/i2c-multi-instantiate.c |Â 2 +-
> > Â drivers/platform/x86/intel_cht_int33fe.cÂÂÂÂ |Â 6 +++---
> > Â 3 files changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> > index 32affd3fa8bd..7e872ceaa14f 100644
> > --- a/drivers/i2c/i2c-core-acpi.c
> > +++ b/drivers/i2c/i2c-core-acpi.c
> > @@ -386,20 +386,22 @@ struct notifier_block i2c_acpi_notifier = {
> > ÂÂ *
> > ÂÂ * Also see i2c_new_device, which this function calls to create the i2c-client.
> > ÂÂ *
> > - * Returns a pointer to the new i2c-client, or NULL if the adapter is not found.
> > + * Returns a pointer to the new i2c-client, or error pointer in case of failure.
> > + * Specifically, -ENODEV is returned if the adapter is not found.
> > ÂÂ */
> > Â struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct i2c_board_info *info)
> > Â {
> > ÂÂÂÂÂ struct i2c_acpi_lookup lookup;
> > ÂÂÂÂÂ struct i2c_adapter *adapter;
> > +ÂÂÂ struct i2c_client *client;
> > ÂÂÂÂÂ struct acpi_device *adev;
> > ÂÂÂÂÂ LIST_HEAD(resource_list);
> > ÂÂÂÂÂ int ret;
> > ÂÂÂÂÂ adev = ACPI_COMPANION(dev);
> > ÂÂÂÂÂ if (!adev)
> > -ÂÂÂÂÂÂÂ return NULL;
> > +ÂÂÂÂÂÂÂ return ERR_PTR(-EINVAL);
> > ÂÂÂÂÂ memset(&lookup, 0, sizeof(lookup));
> > ÂÂÂÂÂ lookup.info = info;
> > @@ -408,16 +410,23 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
> > ÂÂÂÂÂ ret = acpi_dev_get_resources(adev, &resource_list,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ i2c_acpi_fill_info, &lookup);
> > +ÂÂÂ if (ret < 0)
> > +ÂÂÂÂÂÂÂ return ERR_PTR(ret);
> > +
> > ÂÂÂÂÂ acpi_dev_free_resource_list(&resource_list);
> > -ÂÂÂ if (ret < 0 || !info->addr)
> > -ÂÂÂÂÂÂÂ return NULL;
> > +ÂÂÂ if (!info->addr)
> > +ÂÂÂÂÂÂÂ return ERR_PTR(-EADDRNOTAVAIL);
> > ÂÂÂÂÂ adapter = i2c_acpi_find_adapter_by_handle(lookup.adapter_handle);
> > ÂÂÂÂÂ if (!adapter)
> > -ÂÂÂÂÂÂÂ return NULL;
> > +ÂÂÂÂÂÂÂ return ERR_PTR(-ENODEV);
>
> Why not simply return -EPROBE_DEFER here (and simplify the callers a lot).
>
> This is the only case where we really want to defer.
>
>
> > +
> > +ÂÂÂ client = i2c_new_device(adapter, info);
> > +ÂÂÂ if (!client)
> > +ÂÂÂÂÂÂÂ return ERR_PTR(-ENODEV);
>
> If you look at i2c_new_device, it can fail because it is
> out of memory, the i2c slave address is invalid, or
> their already is an i2c slave with the same address,
> iow if this were to return an ERR_PTR itself, this
> would return -ENOMEM, -EINVAL or -EBUSY and never
> -EPROBE_DEFER.
>
> Note keeping the -ENODEV here is fine, what I'm trying
> to say is that in this case the caller of i2c_acpi_new_device
> really should not return PROBE_DEFER, so directly returning
> EPROBE_DEFER above, rather then let the caller guess is better.
>
> And e.g. this:
>
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (IS_ERR(multi->clients[i]))
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(multi->clients[i]);
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ else if (!multi->clients[i])
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ else
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = 0;
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (ret) {
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "Error creating i2c-client, idx %d\n", i);
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto error;
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
>
> Can be simplified to:
>
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (IS_ERR(multi->clients[i])) {
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(multi->clients[i]);
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "Error creating i2c-client, idx %d\n", i);
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto error;
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }

Correction, that should be:

                multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
                if (IS_ERR(multi->clients[i])) {
                        ret = PTR_ERR(multi->clients[i]);
                        if (ret != -EPROBE_DEFER)
                                dev_err(dev, "Error creating i2c-client, idx %d\n", i);
                        goto error;
                }

We don't want to log an error when deferring.

Regards,

Hans



> This will also allow you to split out the patches cleaning up the callers
> without having a bisect problem (the NULL case will now simply never happen).
>
> Regards,
>
> Hans
>
>
> > -ÂÂÂ return i2c_new_device(adapter, info);
> > +ÂÂÂ return client;
> > Â }
> > Â EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
> > diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
> > index 0e3a36f3ef64..18928e4ede7f 100644
> > --- a/drivers/platform/x86/i2c-multi-instantiate.c
> > +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> > @@ -72,7 +72,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ board_info.irq = ret;
> > ÂÂÂÂÂÂÂÂÂ }
> > ÂÂÂÂÂÂÂÂÂ multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
> > -ÂÂÂÂÂÂÂ if (!multi->clients[i])
> > +ÂÂÂÂÂÂÂ if (PTR_ERR(multi->clients[i]) == -ENODEV)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
> > ÂÂÂÂÂÂÂÂÂ else if (IS_ERR(multi->clients[i]))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(multi->clients[i]);
>
>
> > diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> > index 3ba139d3bd03..b8cab17ec596 100644
> > --- a/drivers/platform/x86/intel_cht_int33fe.c
> > +++ b/drivers/platform/x86/intel_cht_int33fe.c
> > @@ -168,7 +168,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
> > ÂÂÂÂÂÂÂÂÂ board_info.dev_name = "max17047";
> > ÂÂÂÂÂÂÂÂÂ board_info.properties = max17047_props;
> > ÂÂÂÂÂÂÂÂÂ data->max17047 = i2c_acpi_new_device(dev, 1, &board_info);
> > -ÂÂÂÂÂÂÂ if (!data->max17047)
> > +ÂÂÂÂÂÂÂ if (PTR_ERR(data->max17047) == -ENODEV)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = -EPROBE_DEFER; /* Wait for i2c-adapter to load */
> > ÂÂÂÂÂÂÂÂÂ else if (IS_ERR(data->max17047))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(data->max17047);
> > @@ -200,7 +200,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
> > ÂÂÂÂÂ board_info.irq = fusb302_irq;
> > ÂÂÂÂÂ data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
> > -ÂÂÂ if (!data->fusb302)
> > +ÂÂÂ if (PTR_ERR(data->fusb302) == -ENODEV)
> > ÂÂÂÂÂÂÂÂÂ ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
> > ÂÂÂÂÂ else if (IS_ERR(data->fusb302))
> > ÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(data->fusb302);
> > @@ -214,7 +214,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
> > ÂÂÂÂÂ strlcpy(board_info.type, "pi3usb30532", I2C_NAME_SIZE);
> > ÂÂÂÂÂ data->pi3usb30532 = i2c_acpi_new_device(dev, 3, &board_info);
> > -ÂÂÂ if (!data->pi3usb30532)
> > +ÂÂÂ if (PTR_ERR(data->pi3usb30532) == -ENODEV)
> > ÂÂÂÂÂÂÂÂÂ ret = -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
> > ÂÂÂÂÂ else if (IS_ERR(data->pi3usb30532))
> > ÂÂÂÂÂÂÂÂÂ ret = PTR_ERR(data->pi3usb30532);