Re: [PATCH v4 2/8] i2c: add I2C Address Translator (ATR) support

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 04/11/2022 14:38, Andy Shevchenko wrote:
> On Fri, Nov 04, 2022 at 01:59:06PM +0200, Tomi Valkeinen wrote:
>> On 01/11/2022 16:30, Andy Shevchenko wrote:
>>> On Tue, Nov 01, 2022 at 03:20:26PM +0200, Tomi Valkeinen wrote:
> 
> ...
> 
>>>> +	ret = atr->ops->attach_client(atr, chan->chan_id, info, client,
>>>> +				      &alias_id);
>>>
>>> On one line looks better.
>>
>> I agree, but it doesn't fit into 80 characters. I personally think that's a
>> too narrow a limit, but some maintainers absolutely require max 80 chars, so
>> I try to limit the lines to 80 unless it looks really ugly.
> 
> OK.
> 
> ...
> 
>>>> +	WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"),
>>>> +	     "can't create symlink to atr device\n");
>>>> +	WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name),
>>>> +	     "can't create symlink for channel %u\n", chan_id);
>>>
>>> Why WARNs? sysfs has already some in their implementation.
>>
>> True, and I can drop these if required. But afaics, sysfs_create_link only
>> warns if there's a duplicate entry, not for other errors.
> 
> The problem with WARN that it can be easily converted to real Oops. Do you
> consider other errors are so fatal that machine would need a reboot?

Yes, WARNs are bad, especially as the error here is not critical. I'll 
change these to dev_warn(). (also, I didn't know WARN could be made to 
oops).

> ...
> 
>>>> +	atr_size = struct_size(atr, adapter, max_adapters);
>>>
>>>> +	if (atr_size == SIZE_MAX)
>>>> +		return ERR_PTR(-EOVERFLOW);
>>>
>>> Dunno if you really need this to be separated from devm_kzalloc(), either way
>>> you will get an error, but in embedded case it will be -ENOMEM.
>>
>> Yep. Well... I kind of like it to be explicit. Calling alloc(SIZE_MAX)
>> doesn't feel nice.
> 
> Yeah, but that is exactly the point of returning SIZE_MAX by the helpers from
> overflow.h. And many of them are called inside a few k*alloc*() APIs.
> 
> So, I don't think it's ugly or not nice from that perspective.

Ok, sounds fine to me. I'll drop the check.

>>>> +	atr = devm_kzalloc(dev, atr_size, GFP_KERNEL);
>>>> +	if (!atr)
>>>> +		return ERR_PTR(-ENOMEM);
> 
> ...
> 
>>>> +EXPORT_SYMBOL_GPL(i2c_atr_delete);
>>>
>>> I would put these to their own namespace from day 1.
>>
>> What would be the namespace? Isn't this something that should be
>> subsystem-wide decision? I have to admit I have never used symbol
>> namespaces, and don't know much about them.
> 
> Yes, subsystem is I2C, but you introducing a kinda subsubsystem. Wouldn't be
> better to provide all symbols in the I2C_ATR namespace from now on?
> 
> It really helps not polluting global namespace and also helps to identify
> users in the source tree.

Alright, I'll look into this.

> ...
> 
>>>> +struct i2c_atr {
>>>> +	/* private: internal use only */
>>>> +
>>>> +	struct i2c_adapter *parent;
>>>> +	struct device *dev;
>>>> +	const struct i2c_atr_ops *ops;
>>>> +
>>>> +	void *priv;
>>>> +
>>>> +	struct i2c_algorithm algo;
>>>> +	struct mutex lock;
>>>> +	int max_adapters;
>>>> +
>>>> +	struct i2c_adapter *adapter[0];
>>>
>>> No VLAs.
>>
>> Ok.
>>
>> I'm not arguing against any of the comments you've made, I think they are
>> all valid, but I want to point out that many of them are in a code copied
>> from i2c-mux.
>>
>> Whether there's any value in keeping i2c-mux and i2c-atr similar in
>> design/style... Maybe not.
> 
> You can address my comment by simply dropping 0 in the respective member.

Oh, I thought you meant no "extensible" structs. I'll drop the 0.

  Tomi