On Mon, 7 Feb 2022 23:55:09 +0100
Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
> On 2/7/22 22:22, Jonathan Cameron wrote:
> > On Mon, 7 Feb 2022 11:34:18 +0100
> > Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >
> >> The mma8452_driver declares both of_match_table and i2c_driver.id_table
> >> match-tables, but its probe() function only checked for of matches.
> >>
> >> Add support for i2c_device_id matches. This fixes the driver not loading
> >> on some x86 tablets (e.g. the Nextbook Ares 8) where the i2c_client is
> >> instantiated by platform code using an i2c_device_id.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > We've lost the fixes tag from the v1 discussion.
>
> Ah right, so that would be:
>
> Fixes: c3cdd6e48e35 ("iio: mma8452: refactor for seperating chip specific data")
>
> I'll add that for v3.
>
> >> ---
> >> Changes in v2:
> >> - Fix the following smatch warning:
> >> drivers/iio/accel/mma8452.c:1595 mma8452_probe() error: we previously assumed 'id' could be null (see line 1536)
> >> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> >> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> >> ---
> >> drivers/iio/accel/mma8452.c | 25 ++++++++++++++++---------
> >> 1 file changed, 16 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> >> index 64b82b4503ad..eaa236cfbabb 100644
> >> --- a/drivers/iio/accel/mma8452.c
> >> +++ b/drivers/iio/accel/mma8452.c
> >> @@ -1523,12 +1523,7 @@ static int mma8452_probe(struct i2c_client *client,
> >> struct iio_dev *indio_dev;
> >> int ret;
> >> const struct of_device_id *match;
> >> -
> >> - match = of_match_device(mma8452_dt_ids, &client->dev);
> >> - if (!match) {
> >> - dev_err(&client->dev, "unknown device model\n");
> >> - return -ENODEV;
> >> - }
> >> + const char *compatible;
> >>
> >> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> >> if (!indio_dev)
> >> @@ -1537,7 +1532,19 @@ static int mma8452_probe(struct i2c_client *client,
> >> data = iio_priv(indio_dev);
> >> data->client = client;
> >> mutex_init(&data->lock);
> >> - data->chip_info = match->data;
> >> +
> >> + if (id) {
> >> + compatible = id->name;
> >> + data->chip_info = &mma_chip_info_table[id->driver_data];
> >> + } else {
> >> + match = of_match_device(mma8452_dt_ids, &client->dev);
> >> + if (!match) {
> >> + dev_err(&client->dev, "unknown device model\n");
> >> + return -ENODEV;
> >> + }
> >> + compatible = match->compatible;
> >
> > Won't this be "fsl,mma8452" or similar when we want "mma8452"?
> > That doesn't matter for the dev_info() but it does matter for
> > indio_dev->name which is part of the userspace ABI.
>
> If we hit the "else" path then yes, this will be e.g. "fsl,mma8452"
> but note how indio_dev->name was previously unconditionally set
> to id->name. This did not cause a NULL ptr deref because the i2c-core
> sets client->name by using of_modalias_node() which strips the
> "fsl," vendor-prefix.
>
> client->name being just "mma8452" means that the id pointer will
> be non NULL even for of-instantiated i2c-clients, instead it
> will point to the "mma8452" i2c_device_id so we will hit the
> if (id) {} path even for of-instantiated i2c-client.
>
> The only case where we can hit the else is if there is an
> entry added to the mma8452_dt_ids[] array which is not present
> in the mma8452_id[] array, which atm is not the case
> (which is confirmed by the lack of bug reports about NULL ptr
> derefs).
>
> Note that since client->name matches on of the mma8452_id[]
> entries the entire mma8452_dt_ids[] array + the else path
> could be completely dropped and things will still work
> through a fallback on only using the mma8452_id[] array.
>
> But AFAIK the preferred method for of enumerated clients
> is to use of-matches.
>
> So ideally I guess we would do something like this:
>
> match = of_match_device(mma8452_dt_ids, &client->dev);
> if (match) {
> compatible = match->compatible;
> data->chip_info = match->data;
> } else if (id) {
> compatible = id->name;
> data->chip_info = &mma_chip_info_table[id->driver_data];
> } else {
> dev_err(&client->dev, "unknown device model\n");
> return -ENODEV;
> }
>
> And then for indio_dev->name do:
>
> indio_dev->name = client->name;
>
> Since that has the vendor-prefix stripped, just like the old
> id->name to which it used to be set.
>
> > Probably easiest way to work around this is to just put the names
> > as an extra entry in the mma_chip_info_table[] so they can
> > be trivially retrieved in either path.
> > Sure it's duplication of a string but they are pretty small and
> > it makes for less special casing in the code.
> >
> > However, looking again at this code I noticed that you haven't
> > actually introduced the fact that id->name wouldn't be set which
> > made me remind myself of how the i2c-core-of.c code works.
> > It has a quirk. It will actually always provide the id via
> > the following path:
> >
> > of_i2c_register_device()
> > -> of_i2c_get_board_info()
> > -> info->type set in of_modalias_node to the part of the compatible after the comma.
> >
> > Then
> > of_i2c_register_device()
> > -> of_i2c_new_client_device()
> > which copies info->type into client->name
> >
> > Then via i2c_device_probe() for the i2c bus the probe is
> > called with an i2c_match_id(driver->id_table, client)
> > to provide the id parameter.
> >
> > So for devicetree you won't hit your else above as if (id)
> > will also pass (which is why the id->name before was working).
> >
> > This path is dropped if we ever move to the probe_new() callback
> > but for now I think it will just work.
> >
> > Now, what to do about this.. In similar cases we do
> > if (client->dev.of_node) {
> > of option.
> > } else {
> > id option
> > }
> > though this is mostly because people don't feel confident
> > the i2c id path will always work for device tree just because
> > (assuming I followed it through correctly) it works today.
>
> I should have read on before replying to your initial remark
> right away, oops. Right as we both say id will be set even for
> of-matches, but only when the of_match and the old i2c_client_id
> match tables both have an entry for the model.
>
> > Now for ACPI there isn't such a path so when we move to
> > generic device properties we can't assume id is anything other
> > than NULL. Note that this driver hasn't previously been converted
> > to generic fw properties because of the absence of a suitable
> > fwnode_irq_get_by_name() but Andy pointed out this week that
> > we now have one available:
> > https://lore.kernel.org/all/YflfEpKj0ilHnQQm@xxxxxxxxxxxxxxxxxx/
> > https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/commit/?h=i2c/alert-for-acpi&id=ca0acb511c21738b32386ce0f85c284b351d919e
> >
> > Given that conversion is likely to happen shortly I'd like to
> > use the pattern above rather than temporarily relying on
> > the struct i2c_device_id always being available.
>
> Hmm, I see, yes my proposed:
>
> indio_dev->name = client->name;
>
> trick will not work once ACPI also comes into play and I see
> that e.g. the bmc150-accel driver already gets the indio_dev->name
> from the chip_info_table[] in the ACPI case, so I will prepare
> a v3 doing so.
>
> I guess that makes this more 5.18 material then 5.17 though,
> but that is fine the platform code instantiating an old
> fashioned i2c-client relying on i2c_client_id matching
> won't land till 5.18 either.
Ah. IN that case I'll yank v3 from the fixes branch over to
the one for next cycle and can apply patch 2 as well.
Thanks,
J
>
> Regards,
>
> Hans
>
>
>
> >> + data->chip_info = match->data;
> >> + }
> >>
> >> data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> >> if (IS_ERR(data->vdd_reg))
> >> @@ -1581,11 +1588,11 @@ static int mma8452_probe(struct i2c_client *client,
> >> }
> >>
> >> dev_info(&client->dev, "registering %s accelerometer; ID 0x%x\n",
> >> - match->compatible, data->chip_info->chip_id);
> >> + compatible, data->chip_info->chip_id);
> >>
> >> i2c_set_clientdata(client, indio_dev);
> >> indio_dev->info = &mma8452_info;
> >> - indio_dev->name = id->name;
> >> + indio_dev->name = compatible;
> >> indio_dev->modes = INDIO_DIRECT_MODE;
> >> indio_dev->channels = data->chip_info->channels;
> >> indio_dev->num_channels = data->chip_info->num_channels;
> >
>
