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

[Tomi Valkeinen]

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