Re: [PATCH v3 2/3] misc: add driver for ST M24LR series RFID/NFC EEPROM chips

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

Hi greg,

Thank you for the detailed feedback.

>> +// SPDX-License-Identifier: GPL-2.0-or-later
>
> Are you sure "or-later" is what you want?  Sorry, I have to ask.
I will remove the or-later part, lol

>> +
>> +#define M24LR_PAGESIZE_DEFAULT	  1u
>> +
>> +#define M24LR_WRITE_TIMEOUT	  25u
>> +#define M24LR_READ_TIMEOUT	  (M24LR_WRITE_TIMEOUT)
>> +
>> +#define to_sys_entry(attrp)   container_of(attrp, struct m24lr_sys_entry, attr)
>
> This shouldn't be needed, something seems odd...

I will remove the M24LR_PAGESIZE_DEFAULT, i do not needed any more
and about the to_sys_entry, i am using it in show and store callbacks

>> +static ssize_t m24lr_ctl_store(struct device *dev, struct device_attribute *attr,
>> +			       const char *buf, size_t count)
>> +{
>> +	struct m24lr *m24lr = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct m24lr_sys_entry *entry = to_sys_entry(attr);
>
> Why isn't this just going off of the device?  Are you using single
> show/store callbacks for multiple attribute types?
>
>> +	unsigned int reg_size = entry->reg_size;
>> +	unsigned int reg_addr = entry->reg_addr;

> Ah, you are.  Are you sure you need/want to do that?

For registers that do not require any special processing, it's sufficient
to directly pass the value to the device. In such cases, a generic store
callback is appropriate. Other registers that require specific handling
have dedicated store callbacks. For example, the unlock attribute uses
its own specialized implementation.

>> +	u8 output[8];
>> +	int err = 0;
>> +
>> +	if (unlikely(!count))
>
> likely/unlikely can ONLY be used when you can benchmark the difference
> in the speed of not having it.  For a sysfs file, that's not needed at
> all, please remove all of these.

Alright, i will do that

>> +		return -EINVAL;
>> +
>> +	if (count > (reg_size << 1))
>> +		return -EINVAL;
>> +
>> +	if (unlikely(!is_power_of_2(reg_size) || reg_size > 8)) {
>> +		dev_dbg(dev,
>> +			"Invalid register size: must be a power of 2 and <= 8 bytes (%u)\n",
>> +			reg_size);
>> +		return -EIO;
>
> Not -EINVAL?  This isn't an I/O error.

The last if statement is primarily for debugging purposes. The reg_size
value is specified internally by the driver (not user-controlled), so
this check helps catch potential mistakes in the driver's sysfs entry
definitions. That's why I used -EIO instead of -EINVAL, as it's not due
to invalid user input but rather an internal misconfiguration.

>> +	n_sss = chip->n_sss_entries;
>> +	if (n_sss) {
>> +		sss = devm_kzalloc(dev, n_sss * sizeof(struct m24lr_sys_entry),
>> +				   GFP_KERNEL);
>> +		if (!sss)
>> +			return -ENOMEM;
>> +
>> +		for (i = 0; i < n_sss; i++) {
>> +			char *name = devm_kasprintf(dev, GFP_KERNEL, "sss%d", i);
>> +
>> +			sss[i].reg_size = 1;
>> +			sss[i].reg_addr = i;
>> +			sss[i].attr.attr.name = name;
>> +			sss[i].attr.attr.mode = 0600;
>> +			sss[i].attr.show = m24lr_ctl_show;
>> +			sss[i].attr.store = m24lr_ctl_store;
>> +
>> +			err = device_create_file(dev, &sss[i].attr);
>
> You just raced with userspace and lost. This is not how to do this,
> please do not dynamically create attributes (hint, this should have
> errored out as you didn't correctly initialize them), but also:

I didn't fully understand where the race condition comes from. Is
the issue caused by calling device_create_file() from within the
probe() function, or is it due to the fact that the attributes
are being allocated dynamically rather than defined statically?

>
>> +			if (err)
>> +				dev_warn(dev,
>> +					 "Failed to create sysfs entry '%s'\n",
>> +					 name);
>
> You do not unwind properly if an error happens.
>
> Just use a default attribute group attached to the driver and the driver
> core will handle all of that logic for you automatically. Making the
> code smaller and even better yet, correct :)

Thanks for clarifying this point. I'll rework the implementation
to use a default attribute_group

Best regards,
Abd-Alrhman Masalkhi