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.
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;
>
