Re: [PATCH] i2c-eg20t: use dynamically registered adapter number

[Jean Delvare <jdelvare@suse.de>]

Hi Wolfram,

On Sun, 18 Sep 2016 21:22:50 +0200, Wolfram Sang wrote:
> 
> > If not, it may make sense to add a helper function exposing
> > __i2c_first_dynamic_bus_num to drivers (something like
> > i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly
> > makes sense if static i2c device definitions exist. If not,
> > i2c_add_adapter() is just as good. So something like:
> > 
> > 	if (i2c_is_dynamic_bus_num(i))
> > 		ret = i2c_add_adapter(pch_adap);
> > 	else {
> > 		pch_adap->nr = i;
> > 		ret = i2c_add_numbered_adapter(pch_adap);
> > 	}
> > 
> > may make sense. Unless someone has a better idea.
> 
> PASEMI does:
> 
>         smbus->adapter.nr = PCI_FUNC(dev->devfn);
> 
> I am unsure if there is any guarantee in what order PCI_FUNCs are
> probed, yet I have the feeling we could try a little harder to get the
> numbered adapter. What about this (untested, just to get the idea)?
> 
> static inline int i2c_add_adapter_try_numbered(struct i2c_adapter *new_adap)
> {
> 	int ret;
> 	struct i2c_adapter *old_adap = i2c_get_adapter(new_adap->nr);
> 
> 	if (old_adap && new_adap->nr >= __i2c_first_dynamic_bus_num) {

Note that __i2c_first_dynamic_bus_num is not currently available to
device driver, so your function can't actually be inlined. This is the
reason why I proposed to introduce i2c_is_dynamic_bus_num(). If you
prefer to expose __i2c_first_dynamic_bus_num directly instead, that's
fine with me, but then you probably want to rename it.

> 		i2c_put_adapter(old_adap);
> 		dev_dbg(&new_adap->dev, "Static bus number occupied, trying dynamic number\n");
> 		ret = i2c_add_adapter(new_adap);
> 	} else {
> 		ret = i2c_add_numbered_adapter(new_adap);

You may be leaking a reference to old_adap here (if old_adap is set but
new_adap->nr < __i2c_first_dynamic_bus_num.)

> 	}
> 
> 	return ret;
> }

I'm a bit confused by the logic, it seems unnecessarily complex to me
(but please keep in mind I am working in a noisy environment these days
so maybe it's just me.)

If old_adap is set, i2c_add_numbered_adapter() has no chance of
working. So why are you additionally comparing with
__i2c_first_dynamic_bus_num? For me you should check either, not both.

My own proposal was checking __i2c_first_dynamic_bus_num. To be honest
I can't see the added value of relying on i2c_get_adapter() instead.
Would the result not be exactly the same? Plus it seems racy, just
because i2c_get_adapter() returns NULL at one point in time doesn't
mean the bus numbers will not have been assigned by the time you call
i2c_add_numbered_adapter().

> I used 'static inline' because I think the drivers needing this should
> carry the extra weight. But no major objection to put sth like this also
> into the core. The documentation for this function should carry a big
> note that this is only a workaround and it should not be used directly.

I don't care much where it goes, to be honest.

If you consider it a workaround, what would be the "real fix" for you?

I was wondering if selecting one of these drivers could set a Kconfig
option to initialize __i2c_first_dynamic_bus_num to a non-zero value.
Unfortunately there does not seem to be a way to set a numeric value to
a Kconfig option using select. We would have to do it indirectly as
with CONFIG_HZ:

choice
	default I2C_RESERVED_BUS_NR_0
	config I2C_RESERVED_BUS_NR_0
	config I2C_RESERVED_BUS_NR_2
endchoice

config I2C_RESERVED_BUS_NR
	int
	default 0 if I2C_RESERVED_BUS_NR_0
	default 2 if I2C_RESERVED_BUS_NR_2

config I2C_EG20T
	tristate "Intel EG20T PCH/LAPIS Semicon IOH(ML7213/ML7223/ML7831) I2C"
	depends on PCI && (X86_32 || MIPS || COMPILE_TEST)
	select I2C_RESERVED_BUS_NR_2

And in i2c-core.c:

__i2c_first_dynamic_bus_num = I2C_RESERVED_BUS_NR;

If that's possible at all... I'm not sure if select works on choice
config options.

Alternatively we could set the default directly based on driver
selection:

config I2C_RESERVED_BUS_NR
	int
	default 0
	default 2 if I2C_EG20T

This is more simple but a little harder to maintain. One possible
problem is if the number of buses isn't known at build time but could
change depending on the hardware, for example. Also I don't know if
more than one such driver can be included in the same kernel.

Or we can make it a user-visible option and leave it on whoever
configures the kernel to get it right. In all cases it would move the
decision to build-time instead of being set dynamically at run-time. 

Note that I am not claiming this is necessarily better than my initial
proposal. I just wanted to mention the possibility.

-- 
Jean Delvare
SUSE L3 Support