Re: [PATCH] i2c: avoid ifdeffery in I2C drivers with optional slave support

Re: [PATCH] i2c: avoid ifdeffery in I2C drivers with optional slave support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

On Wed, Dec 04, 2019 at 10:53:48AM +0100, Sascha Hauer wrote:
> Always add the (un)reg_slave hooks to struct i2c_algorithm, even when
> I2C slave support is disabled. With the cost of some binary space I2C
> drivers with optional I2C slave support no longer have to #ifdef
> the hooks. For the same reason add a stub for i2c_slave_event and make
> enum i2c_slave_event present without I2C slave support.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

This kind of reverts d5fd120e7860 ("i2c: Only include slave support if
selected"), so adding Jean here for more discussion.

I don't mind the additional bytes used in i2c_algorithm, so I am in
favor of this approach.

I do mind the extra bytes used in each and every i2c_client (which is
not affected in this patch). What we could do on top of this:

Because i2c-slave backends will be rare (and only those need it), it
might be worthwhile to introduce a struct i2c_slave_client and embed the
original i2c_client there. Maybe this way, we could get rid of the
I2C_SLAVE symbol entirely? The I2C core code is not a lot as well.

> ---
>  include/linux/i2c.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index d2f786706657..74ebfcb43dd2 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -359,7 +359,6 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
>  
>  /* I2C slave support */
>  
> -#if IS_ENABLED(CONFIG_I2C_SLAVE)
>  enum i2c_slave_event {
>  	I2C_SLAVE_READ_REQUESTED,
>  	I2C_SLAVE_WRITE_REQUESTED,
> @@ -368,6 +367,7 @@ enum i2c_slave_event {
>  	I2C_SLAVE_STOP,
>  }
>  
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
>  extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
>  extern int i2c_slave_unregister(struct i2c_client *client);
>  extern bool i2c_detect_slave_mode(struct device *dev);
> @@ -379,6 +379,11 @@ static inline int i2c_slave_event(struct i2c_client *client,
>  }
>  #else
>  static inline bool i2c_detect_slave_mode(struct device *dev) { return false; }
> +static inline int i2c_slave_event(struct i2c_client *client,
> +				  enum i2c_slave_event event, u8 *val)
> +{
> +	return -EINVAL;
> +}
>  #endif
>  
>  /**
> @@ -553,10 +558,8 @@ struct i2c_algorithm {
>  	/* To determine what the adapter supports */
>  	u32 (*functionality)(struct i2c_adapter *adap);
>  
> -#if IS_ENABLED(CONFIG_I2C_SLAVE)
>  	int (*reg_slave)(struct i2c_client *client);
>  	int (*unreg_slave)(struct i2c_client *client);
> -#endif
>  };
>  
>  /**
> -- 
> 2.24.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: avoid ifdeffery in I2C drivers with optional slave support

[Jean Delvare]

Hi Wolfram, Sascha,

On Thu, 9 Apr 2020 15:40:27 +0200, Wolfram Sang wrote:
> On Wed, Dec 04, 2019 at 10:53:48AM +0100, Sascha Hauer wrote:
> > Always add the (un)reg_slave hooks to struct i2c_algorithm, even when
> > I2C slave support is disabled. With the cost of some binary space I2C
> > drivers with optional I2C slave support no longer have to #ifdef
> > the hooks. For the same reason add a stub for i2c_slave_event and make
> > enum i2c_slave_event present without I2C slave support.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>  
> 
> This kind of reverts d5fd120e7860 ("i2c: Only include slave support if
> selected"), so adding Jean here for more discussion.

That commit made sense then as there was only exactly 1 kernel driver
needing this. This might be revisited when more drivers need it.

That being said, as far as I can see only 8 drivers need it today, which
isn't that many, and more importantly, several architectures will
typically not include support for any of them (i386, x86_64 and s390x
for example).

> I don't mind the additional bytes used in i2c_algorithm, so I am in
> favor of this approach.

I find it questionable to increase the memory footprint on all x86_64
systems out there for a feature they do not need. Sure it's only 16
bytes in one structure, but if every subsystem does the same on a
regular basis, it adds up.

More importantly I can't see how the ifdef'd members of struct
i2c_algorithm are the cause of the problem mentioned by Sascha. He
seems to be concerned by drivers with *optional* I2C slave support
having ifdefs. Why can't this be solved in these drivers directly? What
prevents these drivers from unconditionally selecting I2C_SLAVE if that
makes their code more simple? This moves the overhead decision to the
device driver instead of forcing it to the whole subsystem across all
architectures.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: avoid ifdeffery in I2C drivers with optional slave support

[Sascha Hauer]

On Fri, Apr 10, 2020 at 11:29:14AM +0200, Jean Delvare wrote:
> Hi Wolfram, Sascha,
> 
> On Thu, 9 Apr 2020 15:40:27 +0200, Wolfram Sang wrote:
> > On Wed, Dec 04, 2019 at 10:53:48AM +0100, Sascha Hauer wrote:
> > > Always add the (un)reg_slave hooks to struct i2c_algorithm, even when
> > > I2C slave support is disabled. With the cost of some binary space I2C
> > > drivers with optional I2C slave support no longer have to #ifdef
> > > the hooks. For the same reason add a stub for i2c_slave_event and make
> > > enum i2c_slave_event present without I2C slave support.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>  
> > 
> > This kind of reverts d5fd120e7860 ("i2c: Only include slave support if
> > selected"), so adding Jean here for more discussion.
> 
> That commit made sense then as there was only exactly 1 kernel driver
> needing this. This might be revisited when more drivers need it.
> 
> That being said, as far as I can see only 8 drivers need it today, which
> isn't that many, and more importantly, several architectures will
> typically not include support for any of them (i386, x86_64 and s390x
> for example).
> 
> > I don't mind the additional bytes used in i2c_algorithm, so I am in
> > favor of this approach.
> 
> I find it questionable to increase the memory footprint on all x86_64
> systems out there for a feature they do not need. Sure it's only 16
> bytes in one structure, but if every subsystem does the same on a
> regular basis, it adds up.
> 
> More importantly I can't see how the ifdef'd members of struct
> i2c_algorithm are the cause of the problem mentioned by Sascha. He
> seems to be concerned by drivers with *optional* I2C slave support
> having ifdefs. Why can't this be solved in these drivers directly? What
> prevents these drivers from unconditionally selecting I2C_SLAVE if that
> makes their code more simple? This moves the overhead decision to the
> device driver instead of forcing it to the whole subsystem across all
> architectures.

The drivers could select I2C_SLAVE when they have I2C slave support and
in fact some drivers do this already. This means that we have the
overhead of unneeded I2C slave support when we need that driver in the
Kernel.

I just thought it would be nice to have I2C slave support optional while
still allowing to avoid ifdefs in the driver. Particularly this doesn't
look nice:

 static const struct i2c_algorithm i2c_imx_algo = {
        .master_xfer    = i2c_imx_xfer,
        .functionality  = i2c_imx_func,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+       .reg_slave      = i2c_imx_reg_slave,
+       .unreg_slave    = i2c_imx_unreg_slave,
+#endif
}

The implementation of these functions need ifdefs as well and compile
coverage gets worse.

Yes, we could select I2C_SLAVE from the driver and I don't really care
which way we choose, the space overhead is marginal either way.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] i2c: avoid ifdeffery in I2C drivers with optional slave support

[Jean Delvare]

On Tue, 14 Apr 2020 13:56:00 +0200, Sascha Hauer wrote:
> On Fri, Apr 10, 2020 at 11:29:14AM +0200, Jean Delvare wrote:
> > More importantly I can't see how the ifdef'd members of struct
> > i2c_algorithm are the cause of the problem mentioned by Sascha. He
> > seems to be concerned by drivers with *optional* I2C slave support
> > having ifdefs. Why can't this be solved in these drivers directly? What
> > prevents these drivers from unconditionally selecting I2C_SLAVE if that
> > makes their code more simple? This moves the overhead decision to the
> > device driver instead of forcing it to the whole subsystem across all
> > architectures.  
> 
> The drivers could select I2C_SLAVE when they have I2C slave support and
> in fact some drivers do this already. This means that we have the
> overhead of unneeded I2C slave support when we need that driver in the
> Kernel.

I can't make sense of this statement, sorry. How is I2C slave support
"unneeded" if your kernel includes at least one kernel which needs it?

It is true that I2C slave support is included in the kernel code as
soon as any driver selects I2C_SLAVE, even if that driver is not
currently loaded. The only way around that would be to move the common
code for it to a separate module and all specific members to different,
dedicated structures. But that would in turn cause more overhead for
people who need slave support. The current implementation is the result
of a trade-off decision I made back then. It is the same design goal
which explains why I2C_SMBUS is a separate option: many system classes
do not need it and I did not want to waste memory on these. The
difference of I2C_SMBUS is that it was large and isolated enough to
warrant a separate kernel module altogether.

> I just thought it would be nice to have I2C slave support optional while
> still allowing to avoid ifdefs in the driver. Particularly this doesn't
> look nice:
> 
>  static const struct i2c_algorithm i2c_imx_algo = {
>         .master_xfer    = i2c_imx_xfer,
>         .functionality  = i2c_imx_func,
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +       .reg_slave      = i2c_imx_reg_slave,
> +       .unreg_slave    = i2c_imx_unreg_slave,
> +#endif
> }

Probably a matter of taste, personally I see nothing wrong with it.

> 
> The implementation of these functions need ifdefs as well and compile
> coverage gets worse.

Sorry but you lost me here. How can I2C slave support be "optional" and
at the same time going without ifdefs?

With your patch, device drivers would include slave support even if it
will never be called because that support was not selected at the i2c
core level. That's pretty confusing if you ask me.

It is true that code coverage gets harder with additional configuration
switches, that's the price to pay for being modular.

> Yes, we could select I2C_SLAVE from the driver and I don't really care
> which way we choose, the space overhead is marginal either way.

With the current design, the idea is indeed to let drivers select
I2C_SLAVE when they need it. And I think this solves your problem
nicely as it lets you remove the ifdefs from your driver.

I think it only makes sense to have ifdefs in the driver itself if that
driver can be used on architectures or systems which will never need
slave support and others which will need it, and these systems are
different distribution targets, so that the decision can be made at
build time. If the decision is not going to be made at build time then
the ifdefs only clutter the code and have no value.

More generally I think it is important to have design goals and to
stick to them. The design goal of the current implementation was to let
architectures which do not need slave support have no overhead at all.
This comes at the cost of #ifdefs in the i2c core code and increased
code coverage needs. Usually it should not need #ifdefs in the device
drivers, with the exception of cross-platform devices mentioned above.

If this is no longer desired and we prefer to have more simple core
code and better code coverage at the cost of runtime overhead on
millions of machine, then it can be done but then it must be done
completely, that is, the I2C_SLAVE symbol goes away entirely and slave
support is included in the kernel always, as Wolfram suggested in his
reply. I wouldn't be happy with this move personally but I'm not the
one making the decision and at least it would make sense.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: avoid ifdeffery in I2C drivers with optional slave support

[Sascha Hauer]

On Tue, Apr 14, 2020 at 04:40:09PM +0200, Jean Delvare wrote:
> On Tue, 14 Apr 2020 13:56:00 +0200, Sascha Hauer wrote:
> > On Fri, Apr 10, 2020 at 11:29:14AM +0200, Jean Delvare wrote:
> > > More importantly I can't see how the ifdef'd members of struct
> > > i2c_algorithm are the cause of the problem mentioned by Sascha. He
> > > seems to be concerned by drivers with *optional* I2C slave support
> > > having ifdefs. Why can't this be solved in these drivers directly? What
> > > prevents these drivers from unconditionally selecting I2C_SLAVE if that
> > > makes their code more simple? This moves the overhead decision to the
> > > device driver instead of forcing it to the whole subsystem across all
> > > architectures.  
> > 
> > The drivers could select I2C_SLAVE when they have I2C slave support and
> > in fact some drivers do this already. This means that we have the
> > overhead of unneeded I2C slave support when we need that driver in the
> > Kernel.
> 
> I can't make sense of this statement, sorry. How is I2C slave support
> "unneeded" if your kernel includes at least one kernel which needs it?

I never used I2C slave 
> 
> It is true that I2C slave support is included in the kernel code as
> soon as any driver selects I2C_SLAVE, even if that driver is not
> currently loaded. The only way around that would be to move the common
> code for it to a separate module and all specific members to different,
> dedicated structures. But that would in turn cause more overhead for
> people who need slave support. The current implementation is the result
> of a trade-off decision I made back then. It is the same design goal
> which explains why I2C_SMBUS is a separate option: many system classes
> do not need it and I did not want to waste memory on these. The
> difference of I2C_SMBUS is that it was large and isolated enough to
> warrant a separate kernel module altogether.
> 
> > I just thought it would be nice to have I2C slave support optional while
> > still allowing to avoid ifdefs in the driver. Particularly this doesn't
> > look nice:
> > 
> >  static const struct i2c_algorithm i2c_imx_algo = {
> >         .master_xfer    = i2c_imx_xfer,
> >         .functionality  = i2c_imx_func,
> > +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> > +       .reg_slave      = i2c_imx_reg_slave,
> > +       .unreg_slave    = i2c_imx_unreg_slave,
> > +#endif
> > }
> 
> Probably a matter of taste, personally I see nothing wrong with it.
> 
> > 
> > The implementation of these functions need ifdefs as well and compile
> > coverage gets worse.
> 
> Sorry but you lost me here. How can I2C slave support be "optional" and
> at the same time going without ifdefs?

static int i2c_imx_reg_slave(struct i2c_client *client)
{
	if (!IS_ENABLED(CONFIG_I2C_SLAVE))
		return -ESOMETHING;
	...
}

The code is gone without CONFIG_I2C_SLAVE enabled, yet the compile coverage
is there.

The patch I sent was a suggestion to do it like that. If that's not
wanted I am fine with that and happily select CONFIG_I2C_SLAVE from the
driver entry in Kconfig, or better, suggest Biwen Li
(https://patchwork.kernel.org/patch/11271067/) to do this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] i2c: avoid ifdeffery in I2C drivers with optional slave support

[Jean Delvare]

On Wed, 15 Apr 2020 07:16:19 +0200, Sascha Hauer wrote:
> On Tue, Apr 14, 2020 at 04:40:09PM +0200, Jean Delvare wrote:
> > Sorry but you lost me here. How can I2C slave support be "optional" and
> > at the same time going without ifdefs?  
> 
> static int i2c_imx_reg_slave(struct i2c_client *client)
> {
> 	if (!IS_ENABLED(CONFIG_I2C_SLAVE))
> 		return -ESOMETHING;
> 	...
> }
> 
> The code is gone without CONFIG_I2C_SLAVE enabled, yet the compile coverage
> is there.

OK, *now* I see where you were going from the beginning ;-) And that
makes sense, for drivers which want optional I2C slave support.

Now I think this is down to 2 questions:

1* Does the i2c-imx driver actually need optional slave support, or can
   this support be included unconditionally? Which distributions or
   builds include this driver, and do they typically enable I2C_SLAVE?
   I took a look at the SUSE kernel configs and we already have
   I2C_SLAVE enabled on armv7 and arm64, so it will make no difference
   there. Only our armv6 config includes this driver without I2C_SLAVE,
   so that's the only one for which keeping the slave support optional
   would help. My point is: you stated that you never used slave
   support, and neither did I, but that's not really relevant. What is
   relevant is whether kernels including these drivers are being built
   with I2C_SLAVE or not in practice. If they are then it doesn't
   matter if individual drivers go the conditional or unconditional
   way, unless they add their own Kconfig option to explicitly enable
   slave support (see below).

2* More generally, how many drivers would benefit from your proposed
   change? At the moment I count 8 drivers selecting I2C_SLAVE, 3 of
   these have a separate Kconfig option for including slave support
   which actually makes slave support optional too but in a different
   way. 1 driver (i2c-slave-eeprom) depends on I2C_SLAVE, and 1 driver
   (i2c-aspeed) has #ifdefs for optional slave support. So we have a
   total of 6 drivers which support slave mode unconditionally and 4
   drivers which have conditional support. That doesn't mean that they
   are right doing what they do though. Their authors may have gone for
   unconditional just because they don't like ifdefs, or they may have
   gone for conditional to play it safe. I'm also wondering why the 3
   drivers which have a dedicated Kconfig option
   (I2C_AT91_SLAVE_EXPERIMENTAL, I2C_DESIGNWARE_SLAVE and
   I2C_PXA_SLAVE) did it that way. Is it on purpose because they
   actually want to be able to force slave mode off even if support is
   available at i2c core level? Is it by politeness to not forcibly
   enable slave mode as soon as the driver is enabled? It matters
   because in the former case, your proposed change is of no interest to
   them, while in the latter case it is.

Unfortunately that's a lot of questions in the end and I do not know
the answers nor am I willing to spend time finding them, sorry.

> The patch I sent was a suggestion to do it like that. If that's not
> wanted I am fine with that and happily select CONFIG_I2C_SLAVE from the
> driver entry in Kconfig, or better, suggest Biwen Li
> (https://patchwork.kernel.org/patch/11271067/) to do this.

Well, there are advantages to both approaches, and without answers to
the questions above, I see no reason to favor one or the other. In such
situation I tend to stick to what we have. But of course the decision
is Wolfram's not mine.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: avoid ifdeffery in I2C drivers with optional slave support

[Jean Delvare]

One last thing I meant to mention but forgot...

On Wed, 15 Apr 2020 07:16:19 +0200, Sascha Hauer wrote:
> static int i2c_imx_reg_slave(struct i2c_client *client)
> {
> 	if (!IS_ENABLED(CONFIG_I2C_SLAVE))
> 		return -ESOMETHING;
> 	...
> }
> 
> The code is gone without CONFIG_I2C_SLAVE enabled, yet the compile coverage
> is there.

Compile coverage is nice but it comes at a cost. With the approach
above, the code will be built, then discarded. When the code is
#ifdef'd out, it isn't built at all. This means that your approach,
although it has advantages, increases the build time.

And don't tell me "you only build once", we live at the time of
continuous integration so we keep building kernels. As a support
engineer, I build kernels daily, and even though I have access to a
powerful build farm for that purpose, it still takes 30 to 60 minutes
to get my kernel built each time. Obviously most of that time isn't
spent on the i2c-imx driver ;-) but if every piece of code does the
same, build time will inevitably increase.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: avoid ifdeffery in I2C drivers with optional slave support

[Sascha Hauer]

On Fri, Apr 17, 2020 at 04:00:14PM +0200, Jean Delvare wrote:
> One last thing I meant to mention but forgot...
> 
> On Wed, 15 Apr 2020 07:16:19 +0200, Sascha Hauer wrote:
> > static int i2c_imx_reg_slave(struct i2c_client *client)
> > {
> > 	if (!IS_ENABLED(CONFIG_I2C_SLAVE))
> > 		return -ESOMETHING;
> > 	...
> > }
> > 
> > The code is gone without CONFIG_I2C_SLAVE enabled, yet the compile coverage
> > is there.
> 
> Compile coverage is nice but it comes at a cost. With the approach
> above, the code will be built, then discarded. When the code is
> #ifdef'd out, it isn't built at all. This means that your approach,
> although it has advantages, increases the build time.
> 
> And don't tell me "you only build once", we live at the time of
> continuous integration so we keep building kernels. As a support
> engineer, I build kernels daily, and even though I have access to a
> powerful build farm for that purpose, it still takes 30 to 60 minutes
> to get my kernel built each time. Obviously most of that time isn't
> spent on the i2c-imx driver ;-) but if every piece of code does the
> same, build time will inevitably increase.

Well often enough I spend more time building Kernels than actually
running them, so don't tell me.

Yes, build time increases when we do things in C rather than in CPP,
but each #ifdef doubles the number of builds necessary to let the
compiler go though all paths. I've sent and received more than enough
patches that fix #ifdefs for the more unlikely cases which were not
compile tested.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |