Silead DMI driver

Silead DMI driver

[Jean Delvare]

Hi all,

I'm looking at drivers/platform/x86/silead_dmi.c which is being added
to kernel v4.11 and I do not like what I see.

This driver is for very specific hardware. Most users won't need it.
Still this driver can only be built into the kernel, no modular option
available.

Apparently this first issue is caused by the necessity to add the
notifier at a specific point in the init sequence. Do I understand
correctly that the I2C device will be instantiated by the ACPI core as
it enumerates devices found in the ACPI tables, and you want to add the
missing properties at that exact point of the init sequence?

As a side note, the comment about the init sequence says: "after i2c
core is initialized and i2c bus itself is ready" but I don't think
there is any actual guarantee on the latter. I see no dependency on
the i2c bus driver, only on the i2c core. What i2c bus driver are we
talking about and how can you be sure it is already loaded at that
point? (Not that it should matter but I'm curious.)

To make things worse, I see that this driver registers a bus notifier
unconditionally. So it will be registered on virtually _every_ x86
system out there, and will kick in and perform DMI checks for every I2C
device being added, even though most people don't need it at all. Is
there any excuse for not checking the DMI data _before_ registering the
bus notifier, so the whole thing can be skipped on the 99% of systems
where it is irrelevant? That wouldn't solve the undue memory
consumption but at least it would limit the impact on boot time.

I have to say I don't understand the whole complexity of the design. As
I understand it, the properties which are being added are only consumed
by the "silead" touchscreen driver. I see no necessity to add the
missing properties before that driver is even loaded. Can't you just
look for the ACPI companion device at the time the silead driver tries
to bind to the i2c device, and add the missing properties before
performing the actual probe? This would be so much simpler. What am I
missing?

PS: I hope this code will die, but if it stays, I see that you call
to_i2c_client() on the notified device without checking if it is an
i2c_client. It could be an i2c_adapter instead, and then you are
forcibly mapping a struct i2c_client on top of a struct i2c_adapter.
Good luck when you will access client->name a few lines later...
i2c_verify_client() is what you want to use instead of to_i2c_client()
in that situtation.

-- 
Jean Delvare
SUSE L3 Support

Re: Silead DMI driver

[Andy Shevchenko]

On Mon, 2017-04-24 at 13:23 +0200, Jean Delvare wrote:
> Hi all,
> 
> I'm looking at drivers/platform/x86/silead_dmi.c which is being added
> to kernel v4.11 and I do not like what I see.

I don't like it either by some other reasons.

> I have to say I don't understand the whole complexity of the design.
> As
> I understand it, the properties which are being added are only
> consumed
> by the "silead" touchscreen driver. I see no necessity to add the
> missing properties before that driver is even loaded. Can't you just
> look for the ACPI companion device at the time the silead driver tries
> to bind to the i2c device, and add the missing properties before
> performing the actual probe? This would be so much simpler. What am I
> missing?

As far as I understand it would be as simple as adding a quirk in actual
driver (touchscreen), but there is strong objection of adding quirks to
the drivers/input/* from Dmitry as I noticed during discussion [1] about
GPIO ACPI library fixes I'm working on.

[1] https://lkml.org/lkml/2017/4/4/593

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: Silead DMI driver

[Jean Delvare]

On Mon, 24 Apr 2017 14:48:16 +0300, Andy Shevchenko wrote:
> On Mon, 2017-04-24 at 13:23 +0200, Jean Delvare wrote:
> > I'm looking at drivers/platform/x86/silead_dmi.c which is being added
> > to kernel v4.11 and I do not like what I see.
> 
> I don't like it either by some other reasons.
> 
> > I have to say I don't understand the whole complexity of the design.
> > As I understand it, the properties which are being added are only
> > consumed by the "silead" touchscreen driver. I see no necessity to add
> > the missing properties before that driver is even loaded. Can't you
> > just look for the ACPI companion device at the time the silead driver
> > tries to bind to the i2c device, and add the missing properties before
> > performing the actual probe? This would be so much simpler. What am I
> > missing?
> 
> As far as I understand it would be as simple as adding a quirk in actual
> driver (touchscreen), but there is strong objection of adding quirks to
> the drivers/input/* from Dmitry as I noticed during discussion [1] about
> GPIO ACPI library fixes I'm working on.
> 
> [1] https://lkml.org/lkml/2017/4/4/593

Thanks for the pointer Andy. OK, I can understand the argument of
Dmitry that platform-specific quirks do not belong to the device
drivers, even though in practice I'm not sure the cost of having a
separate platform driver for the purpose is always worth it - depends
on how "popular" the device is, I suppose.

But still, this can't justify non-modular platform code that will run
on every X86 system out there. Any piece of platform-specific quirks,
we should be able to build as a module, otherwise it simply doesn't
scale. PCI quirks and such are enough pain already without inventing
more flavors of bloat :-(

-- 
Jean Delvare
SUSE L3 Support

Re: Silead DMI driver

[Darren Hart]

On Tue, Apr 25, 2017 at 12:19:39PM +0200, Jean Delvare wrote:
> On Mon, 24 Apr 2017 14:48:16 +0300, Andy Shevchenko wrote:
> > On Mon, 2017-04-24 at 13:23 +0200, Jean Delvare wrote:
> > > I'm looking at drivers/platform/x86/silead_dmi.c which is being added
> > > to kernel v4.11 and I do not like what I see.
> > 
> > I don't like it either by some other reasons.
> > 
> > > I have to say I don't understand the whole complexity of the design.
> > > As I understand it, the properties which are being added are only
> > > consumed by the "silead" touchscreen driver. I see no necessity to add
> > > the missing properties before that driver is even loaded. Can't you
> > > just look for the ACPI companion device at the time the silead driver
> > > tries to bind to the i2c device, and add the missing properties before
> > > performing the actual probe? This would be so much simpler. What am I
> > > missing?
> > 
> > As far as I understand it would be as simple as adding a quirk in actual
> > driver (touchscreen), but there is strong objection of adding quirks to
> > the drivers/input/* from Dmitry as I noticed during discussion [1] about
> > GPIO ACPI library fixes I'm working on.
> > 
> > [1] https://lkml.org/lkml/2017/4/4/593
> 
> Thanks for the pointer Andy. OK, I can understand the argument of
> Dmitry that platform-specific quirks do not belong to the device
> drivers, even though in practice I'm not sure the cost of having a
> separate platform driver for the purpose is always worth it - depends
> on how "popular" the device is, I suppose.
> 
> But still, this can't justify non-modular platform code that will run
> on every X86 system out there. Any piece of platform-specific quirks,
> we should be able to build as a module, otherwise it simply doesn't
> scale. PCI quirks and such are enough pain already without inventing
> more flavors of bloat :-(
> 

Jean makes a good point. I would insist on this if the Kconfig default was y or
if distros were likely to enable this by default. However, this is for the still
very rare x86 tablet and is likely to only be enabled for those systems which
require it.

To emphasize its role, perhaps this driver should have a depends on TOUCHSCREEN_SILEAD?

While the default for Kconfig is n if not explicitly set to m or y, perhaps
"default n" should be added to TOUCHSCREEN_SILEAD and SILEAD_DMI to make it more
explicit. I don't know if there is an official preference on "default n"
statements or not.

-- 
Darren Hart
VMware Open Source Technology Center

Re: Silead DMI driver

[Dmitry Torokhov]

On Thu, Apr 27, 2017 at 02:13:22PM -0700, Darren Hart wrote:
> On Tue, Apr 25, 2017 at 12:19:39PM +0200, Jean Delvare wrote:
> > On Mon, 24 Apr 2017 14:48:16 +0300, Andy Shevchenko wrote:
> > > On Mon, 2017-04-24 at 13:23 +0200, Jean Delvare wrote:
> > > > I'm looking at drivers/platform/x86/silead_dmi.c which is being added
> > > > to kernel v4.11 and I do not like what I see.
> > > 
> > > I don't like it either by some other reasons.
> > > 
> > > > I have to say I don't understand the whole complexity of the design.
> > > > As I understand it, the properties which are being added are only
> > > > consumed by the "silead" touchscreen driver. I see no necessity to add
> > > > the missing properties before that driver is even loaded. Can't you
> > > > just look for the ACPI companion device at the time the silead driver
> > > > tries to bind to the i2c device, and add the missing properties before
> > > > performing the actual probe? This would be so much simpler. What am I
> > > > missing?
> > > 
> > > As far as I understand it would be as simple as adding a quirk in actual
> > > driver (touchscreen), but there is strong objection of adding quirks to
> > > the drivers/input/* from Dmitry as I noticed during discussion [1] about
> > > GPIO ACPI library fixes I'm working on.
> > > 
> > > [1] https://lkml.org/lkml/2017/4/4/593
> > 
> > Thanks for the pointer Andy. OK, I can understand the argument of
> > Dmitry that platform-specific quirks do not belong to the device
> > drivers, even though in practice I'm not sure the cost of having a
> > separate platform driver for the purpose is always worth it - depends
> > on how "popular" the device is, I suppose.
> > 
> > But still, this can't justify non-modular platform code that will run
> > on every X86 system out there. Any piece of platform-specific quirks,
> > we should be able to build as a module, otherwise it simply doesn't
> > scale. PCI quirks and such are enough pain already without inventing
> > more flavors of bloat :-(
> > 
> 
> Jean makes a good point. I would insist on this if the Kconfig default was y or
> if distros were likely to enable this by default. However, this is for the still
> very rare x86 tablet and is likely to only be enabled for those systems which
> require it.
> 
> To emphasize its role, perhaps this driver should have a depends on TOUCHSCREEN_SILEAD?

Yeah, that makes sense to me.

> 
> While the default for Kconfig is n if not explicitly set to m or y, perhaps
> "default n" should be added to TOUCHSCREEN_SILEAD and SILEAD_DMI to make it more
> explicit. I don't know if there is an official preference on "default n"
> statements or not.
> 

Thanks.

-- 
Dmitry

Re: Silead DMI driver

[Jean Delvare]

Hi Darren,

On Thu, 27 Apr 2017 14:13:22 -0700, Darren Hart wrote:
> On Tue, Apr 25, 2017 at 12:19:39PM +0200, Jean Delvare wrote:
> > Thanks for the pointer Andy. OK, I can understand the argument of
> > Dmitry that platform-specific quirks do not belong to the device
> > drivers, even though in practice I'm not sure the cost of having a
> > separate platform driver for the purpose is always worth it - depends
> > on how "popular" the device is, I suppose.
> > 
> > But still, this can't justify non-modular platform code that will run
> > on every X86 system out there. Any piece of platform-specific quirks,
> > we should be able to build as a module, otherwise it simply doesn't
> > scale. PCI quirks and such are enough pain already without inventing
> > more flavors of bloat :-(
> 
> Jean makes a good point. I would insist on this if the Kconfig default was y or
> if distros were likely to enable this by default. However, this is for the still
> very rare x86 tablet and is likely to only be enabled for those systems which
> require it.

The problem is that there is no clear line of what hardware openSUSE
kernels should support or not. I don't know if other general purpose
Linux distributions have made any statement on the matter, but we have
not. As a matter of fact, the person in charge of deciding what to do
with new kernel drivers did enable both Silead touchscreen options in
our standard i386 and x86_64 kernels.

There are also people working on getting openSUSE to run on odd
hardware such as the GPD Win, and we have ARM kernels running on
embedded platforms. As X86-based embedded platforms are getting
popular, what sense would it make to support everything from X86
servers to embedded ARM but rule out X86 tablets? Especially when
hybrid laptop/tablet designs are becoming more popular as well. The
same touchscreen device used for pure tablets may be used for hybrid
models tomorrow.

This is one of the reasons why modular drivers are a must. If we enable
them when we shouldn't have, the cost stays low. Of course it would be
better if we could just enable what we really need, but in practice
this is terribly hard to achieve. This is also the reason why I am
actively promoting the mentioning of intended target hardware on new
drivers, to limit the risks of us enabling drivers on kernels that
don't need it.

> (...)
> While the default for Kconfig is n if not explicitly set to m or y, perhaps
> "default n" should be added to TOUCHSCREEN_SILEAD and SILEAD_DMI to make it more
> explicit. I don't know if there is an official preference on "default n"
> statements or not.

I wondered before as well. But given that n is the default default, and
an explicit default is not listed by "make config" nor "make
menuconfig", I can't see the value of "default n". It makes no
difference for the user. In fact my never-ending project list has a
point "Kconfig: kill all default n and add a test to checkpatch so they
don't come back". Chances that I get to that one before retirement are
low though.

I am not convinced that a default value for a device driver makes any
sense in the first place. If you don't need support for the device, you
don't enable the driver, period. I can't see how a default answer can
be relevant. In this specific case, the default answer should be n if
not building a kernel for tablets. But would become y if building a
kernel for tablets. What reason would there be to favor one scenario
over the other?

I believe default values are only useful for driver feature options,
general kernel configuration options, or automatic selection of hidden
options.

-- 
Jean Delvare
SUSE L3 Support

Re: Silead DMI driver

[Darren Hart]

On Wed, May 03, 2017 at 10:19:53AM +0200, Jean Delvare wrote:
> Hi Darren,
> 
> On Thu, 27 Apr 2017 14:13:22 -0700, Darren Hart wrote:
> > On Tue, Apr 25, 2017 at 12:19:39PM +0200, Jean Delvare wrote:
> > > Thanks for the pointer Andy. OK, I can understand the argument of
> > > Dmitry that platform-specific quirks do not belong to the device
> > > drivers, even though in practice I'm not sure the cost of having a
> > > separate platform driver for the purpose is always worth it - depends
> > > on how "popular" the device is, I suppose.
> > > 
> > > But still, this can't justify non-modular platform code that will run
> > > on every X86 system out there. Any piece of platform-specific quirks,
> > > we should be able to build as a module, otherwise it simply doesn't
> > > scale. PCI quirks and such are enough pain already without inventing
> > > more flavors of bloat :-(
> > 
> > Jean makes a good point. I would insist on this if the Kconfig default was y or
> > if distros were likely to enable this by default. However, this is for the still
> > very rare x86 tablet and is likely to only be enabled for those systems which
> > require it.
> 
> The problem is that there is no clear line of what hardware openSUSE
> kernels should support or not. I don't know if other general purpose
> Linux distributions have made any statement on the matter, but we have
> not. As a matter of fact, the person in charge of deciding what to do
> with new kernel drivers did enable both Silead touchscreen options in
> our standard i386 and x86_64 kernels.

I'd assert that wasn't the right call. Happy to discuss with them if they are
interested.

> 
> There are also people working on getting openSUSE to run on odd
> hardware such as the GPD Win, and we have ARM kernels running on
> embedded platforms. As X86-based embedded platforms are getting
> popular, what sense would it make to support everything from X86
> servers to embedded ARM but rule out X86 tablets? Especially when
> hybrid laptop/tablet designs are becoming more popular as well. The
> same touchscreen device used for pure tablets may be used for hybrid
> models tomorrow.

I suppose it depends what you mean by support. If you mean have one kernel that
runs on everything from embedded dev boards to HPC, I think you'll find that to
be an untenable goal. In my time maintaining the Yocto Project BSPs for Intel,
it is still the case that embedded systems prefer to run more tailored kernels.
We tried to reduce that change as much as possible on IA, by at least building
from the same source branch, but ultimately, people want to change configuration
options on embedded systems.

> 
> This is one of the reasons why modular drivers are a must. If we enable
> them when we shouldn't have, the cost stays low. Of course it would be
> better if we could just enable what we really need, but in practice
> this is terribly hard to achieve. This is also the reason why I am
> actively promoting the mentioning of intended target hardware on new
> drivers, to limit the risks of us enabling drivers on kernels that
> don't need it.
> 

This works great for Desktops and Datacenters, but it doesn't hold up very long
in discussions with the more embedded and special purpose device space. 

> > (...)
> > While the default for Kconfig is n if not explicitly set to m or y, perhaps
> > "default n" should be added to TOUCHSCREEN_SILEAD and SILEAD_DMI to make it more
> > explicit. I don't know if there is an official preference on "default n"
> > statements or not.
> 
> I wondered before as well. But given that n is the default default, and
> an explicit default is not listed by "make config" nor "make
> menuconfig", I can't see the value of "default n". It makes no
> difference for the user. In fact my never-ending project list has a
> point "Kconfig: kill all default n and add a test to checkpatch so they
> don't come back". Chances that I get to that one before retirement are
> low though.

:-) Agreed on all points

> 
> I am not convinced that a default value for a device driver makes any
> sense in the first place. If you don't need support for the device, you
> don't enable the driver, period. I can't see how a default answer can
> be relevant. In this specific case, the default answer should be n if
> not building a kernel for tablets. But would become y if building a
> kernel for tablets. What reason would there be to favor one scenario
> over the other?

The liklihood of use is the most compelling argument. e1000 is very likely to be
used relative to silead-touchscreen, for example. If it's highly specialized, it
should typically be default n (or not default y|m as discussed above). Of course
people like you ultimately are on the hook to make that call.

> 
> I believe default values are only useful for driver feature options,
> general kernel configuration options, or automatic selection of hidden
> options.

I can see that view, especiacally coming from your position.

-- 
Darren Hart
VMware Open Source Technology Center

Re: Silead DMI driver

[Dmitry Torokhov]

Hi Jean,

On Mon, Apr 24, 2017 at 01:23:56PM +0200, Jean Delvare wrote:
> Hi all,
> 
> I'm looking at drivers/platform/x86/silead_dmi.c which is being added
> to kernel v4.11 and I do not like what I see.
> 
> This driver is for very specific hardware. Most users won't need it.
> Still this driver can only be built into the kernel, no modular option
> available.
> 
> Apparently this first issue is caused by the necessity to add the
> notifier at a specific point in the init sequence. Do I understand
> correctly that the I2C device will be instantiated by the ACPI core as
> it enumerates devices found in the ACPI tables, and you want to add the
> missing properties at that exact point of the init sequence?

Yes.

> 
> As a side note, the comment about the init sequence says: "after i2c
> core is initialized and i2c bus itself is ready" but I don't think
> there is any actual guarantee on the latter. I see no dependency on
> the i2c bus driver, only on the i2c core. What i2c bus driver are we
> talking about and how can you be sure it is already loaded at that
> point? (Not that it should matter but I'm curious.)

We are talking about the i2c bus in the Linux driver model sense, i.e.

	extern struct bus_type i2c_bus_type;

No I2C hardware is needed at this time.

> 
> To make things worse, I see that this driver registers a bus notifier
> unconditionally. So it will be registered on virtually _every_ x86
> system out there, and will kick in and perform DMI checks for every I2C
> device being added, even though most people don't need it at all. Is
> there any excuse for not checking the DMI data _before_ registering the
> bus notifier, so the whole thing can be skipped on the 99% of systems
> where it is irrelevant? That wouldn't solve the undue memory
> consumption but at least it would limit the impact on boot time.

If you take a look at next you will see that we no longer register bus
notifier unconditionally. I also thought that Hans was going to annotate
most of the items as __initconst so that unneeded memory can be
discarded after boot, but for that some other patches to device
properties should trickle through various subsystems. In the end you'll
end up with just the properties for the board you need.

> 
> I have to say I don't understand the whole complexity of the design. As
> I understand it, the properties which are being added are only consumed
> by the "silead" touchscreen driver. I see no necessity to add the
> missing properties before that driver is even loaded. Can't you just
> look for the ACPI companion device at the time the silead driver tries
> to bind to the i2c device, and add the missing properties before
> performing the actual probe? This would be so much simpler. What am I
> missing?

So the idea is that I am trying to make drivers in input/
platform-independent, and move platform quirks out of them. With generic
device properties we can finally move away from ad-hoc platform data,
and rely on proper bindings, and that ensures at least some engineering
control.

So while looking for the ACPI companion device might be indeed very
easy, it pushes dependencies on ACPI and platform knowledge into the
generic driver, which I am trying to avoid. I believe
drivers/platform/x86/ is a good place to stash all x86 platform quirks.

I agree that tying this up with I2C bus notifier might not be the best
solution overall (it was simply available tight at this time) and we
could look into hooking this up into ACPI device enumeration to supply
missing properties.

> 
> PS: I hope this code will die, but if it stays, I see that you call
> to_i2c_client() on the notified device without checking if it is an
> i2c_client. It could be an i2c_adapter instead, and then you are
> forcibly mapping a struct i2c_client on top of a struct i2c_adapter.
> Good luck when you will access client->name a few lines later...
> i2c_verify_client() is what you want to use instead of to_i2c_client()
> in that situtation.

That is also should be fixed in -next. Because i2c_adapter structure is
big enough so that looking up client->name in adapter is not going to
blow up, nor will it likely to match, it was decided it could stay till
next merge window.

Thanks.

-- 
Dmitry

Re: Silead DMI driver

[Jean Delvare]

Hi Dmitry,

On Mon, 24 Apr 2017 09:59:35 -0700, Dmitry Torokhov wrote:
> On Mon, Apr 24, 2017 at 01:23:56PM +0200, Jean Delvare wrote:
> > As a side note, the comment about the init sequence says: "after i2c
> > core is initialized and i2c bus itself is ready" but I don't think
> > there is any actual guarantee on the latter. I see no dependency on
> > the i2c bus driver, only on the i2c core. What i2c bus driver are we
> > talking about and how can you be sure it is already loaded at that
> > point? (Not that it should matter but I'm curious.)
> 
> We are talking about the i2c bus in the Linux driver model sense, i.e.
> 
> 	extern struct bus_type i2c_bus_type;
> 
> No I2C hardware is needed at this time.

Thanks for the clarification. The wording is misleading then, in my
views at least, i2c_bus_type belongs to the i2c core, so "after i2c
core is initialized" would be enough.

> > To make things worse, I see that this driver registers a bus notifier
> > unconditionally. So it will be registered on virtually _every_ x86
> > system out there, and will kick in and perform DMI checks for every I2C
> > device being added, even though most people don't need it at all. Is
> > there any excuse for not checking the DMI data _before_ registering the
> > bus notifier, so the whole thing can be skipped on the 99% of systems
> > where it is irrelevant? That wouldn't solve the undue memory
> > consumption but at least it would limit the impact on boot time.
> 
> If you take a look at next you will see that we no longer register bus
> notifier unconditionally.

I did not think of looking there, my bad. Indeed I see this is already
fixed, which is good.

> I also thought that Hans was going to annotate
> most of the items as __initconst so that unneeded memory can be
> discarded after boot, but for that some other patches to device
> properties should trickle through various subsystems. In the end you'll
> end up with just the properties for the board you need.

That's better, but won't you still have to live with
silead_ts_dmi_notifier_call() and silead_ts_dmi_add_props() loaded
forever, and the notification hook enabled,  even if you don't have any
Silead touchscreen on your x86 system? I can't see how a
notification-based implementation could clean up everything, as you
supposedly don't know when you will be called (which is my main concern
about this implementation - you should be able to run the fixup code
once and be done with it.)

> > I have to say I don't understand the whole complexity of the design. As
> > I understand it, the properties which are being added are only consumed
> > by the "silead" touchscreen driver. I see no necessity to add the
> > missing properties before that driver is even loaded. Can't you just
> > look for the ACPI companion device at the time the silead driver tries
> > to bind to the i2c device, and add the missing properties before
> > performing the actual probe? This would be so much simpler. What am I
> > missing?
> 
> So the idea is that I am trying to make drivers in input/
> platform-independent, and move platform quirks out of them. With generic
> device properties we can finally move away from ad-hoc platform data,
> and rely on proper bindings, and that ensures at least some engineering
> control.
> 
> So while looking for the ACPI companion device might be indeed very
> easy, it pushes dependencies on ACPI and platform knowledge into the
> generic driver, which I am trying to avoid. I believe
> drivers/platform/x86/ is a good place to stash all x86 platform quirks.

While this is a laudable goal, how will it scale in practice? I mean,
most drivers do need quirks in one form or another. Are we really going
to double the number of "drivers" (as in .c files and modules) and
Kconfig options to cleanly separate quirks from the device drivers
proper?

>From my perspective, you are solving the problem that all users of a
driver shouldn't have to pay for platforms which did get things wrong,
but your solution causes all users of the platform to pay the overhead
instead. Which is statistically a much larger number of people. If we
are going to be unfair, we might as well go for the most simple
solution.

So I'm sorry but I can't see how your approach is better than just
ifdef'd platform-specific code in the device driver, as we have been
doing so far.

> I agree that tying this up with I2C bus notifier might not be the best
> solution overall (it was simply available tight at this time) and we
> could look into hooking this up into ACPI device enumeration to supply
> missing properties.

I have a hard time understanding how you did not come up with this idea
first, to be honest. If the problem you need to solve is caused by
missing data in the ACPI tables, injecting the missing data at the ACPI
level would seem to be the most straightforward way to go. Fixing the
problem as close as possible to its source. But I will admit I am not
familiar enough with the ACPI subsystem implementation to say if that
can be done today or if preparatory work is needed.

> > PS: I hope this code will die, but if it stays, I see that you call
> > to_i2c_client() on the notified device without checking if it is an
> > i2c_client. It could be an i2c_adapter instead, and then you are
> > forcibly mapping a struct i2c_client on top of a struct i2c_adapter.
> > Good luck when you will access client->name a few lines later...
> > i2c_verify_client() is what you want to use instead of to_i2c_client()
> > in that situtation.
> 
> That is also should be fixed in -next. Because i2c_adapter structure is
> big enough so that looking up client->name in adapter is not going to
> blow up, nor will it likely to match, it was decided it could stay till
> next merge window.

I'm speechless. If you believe that this kind of bug isn't worth fixing
quickly, I'll go disable that piece of cra^H^Hode from the opeSUSE
kernel for the time being. And then, go figure if/when anyone will
remember to re-enable it once it is in an acceptable state.

I thought the rule about upstream kernel code acceptance was that you
should get things right first, no matter how much time and how many
iterations it takes, and then we commit the result. Apparently this has
changed while I was not looking :-(

-- 
Jean Delvare
SUSE L3 Support

Re: Silead DMI driver

[Dmitry Torokhov]

Hi Jean,

On Fri, Apr 28, 2017 at 11:33:21AM +0200, Jean Delvare wrote:
> Hi Dmitry,
> 
> On Mon, 24 Apr 2017 09:59:35 -0700, Dmitry Torokhov wrote:
> > On Mon, Apr 24, 2017 at 01:23:56PM +0200, Jean Delvare wrote:
> > > As a side note, the comment about the init sequence says: "after i2c
> > > core is initialized and i2c bus itself is ready" but I don't think
> > > there is any actual guarantee on the latter. I see no dependency on
> > > the i2c bus driver, only on the i2c core. What i2c bus driver are we
> > > talking about and how can you be sure it is already loaded at that
> > > point? (Not that it should matter but I'm curious.)
> > 
> > We are talking about the i2c bus in the Linux driver model sense, i.e.
> > 
> > 	extern struct bus_type i2c_bus_type;
> > 
> > No I2C hardware is needed at this time.
> 
> Thanks for the clarification. The wording is misleading then, in my
> views at least, i2c_bus_type belongs to the i2c core, so "after i2c
> core is initialized" would be enough.

OK.

> 
> > > To make things worse, I see that this driver registers a bus notifier
> > > unconditionally. So it will be registered on virtually _every_ x86
> > > system out there, and will kick in and perform DMI checks for every I2C
> > > device being added, even though most people don't need it at all. Is
> > > there any excuse for not checking the DMI data _before_ registering the
> > > bus notifier, so the whole thing can be skipped on the 99% of systems
> > > where it is irrelevant? That wouldn't solve the undue memory
> > > consumption but at least it would limit the impact on boot time.
> > 
> > If you take a look at next you will see that we no longer register bus
> > notifier unconditionally.
> 
> I did not think of looking there, my bad. Indeed I see this is already
> fixed, which is good.
> 
> > I also thought that Hans was going to annotate
> > most of the items as __initconst so that unneeded memory can be
> > discarded after boot, but for that some other patches to device
> > properties should trickle through various subsystems. In the end you'll
> > end up with just the properties for the board you need.
> 
> That's better, but won't you still have to live with
> silead_ts_dmi_notifier_call() and silead_ts_dmi_add_props() loaded
> forever, and the notification hook enabled,  even if you don't have any
> Silead touchscreen on your x86 system? I can't see how a
> notification-based implementation could clean up everything, as you
> supposedly don't know when you will be called (which is my main concern
> about this implementation - you should be able to run the fixup code
> once and be done with it.)

Yes, I agree that discarding everything would be better approach. I can
see if this all can be shifted to ACPI and away form i2c. We'll need
something similar for atmel devices on some chromebooks too.

> 
> > > I have to say I don't understand the whole complexity of the design. As
> > > I understand it, the properties which are being added are only consumed
> > > by the "silead" touchscreen driver. I see no necessity to add the
> > > missing properties before that driver is even loaded. Can't you just
> > > look for the ACPI companion device at the time the silead driver tries
> > > to bind to the i2c device, and add the missing properties before
> > > performing the actual probe? This would be so much simpler. What am I
> > > missing?
> > 
> > So the idea is that I am trying to make drivers in input/
> > platform-independent, and move platform quirks out of them. With generic
> > device properties we can finally move away from ad-hoc platform data,
> > and rely on proper bindings, and that ensures at least some engineering
> > control.
> > 
> > So while looking for the ACPI companion device might be indeed very
> > easy, it pushes dependencies on ACPI and platform knowledge into the
> > generic driver, which I am trying to avoid. I believe
> > drivers/platform/x86/ is a good place to stash all x86 platform quirks.
> 
> While this is a laudable goal, how will it scale in practice? I mean,
> most drivers do need quirks in one form or another. Are we really going
> to double the number of "drivers" (as in .c files and modules) and
> Kconfig options to cleanly separate quirks from the device drivers
> proper?

I do not see minimizing number of drivers as a goal. I also expect that
devices that get one driver wrong would require more fixups. As far as
having multiple config options - you do realize that even if we move
these chunks into existing drivers they can still be protected by config
options?

> 
> From my perspective, you are solving the problem that all users of a
> driver shouldn't have to pay for platforms which did get things wrong,
> but your solution causes all users of the platform to pay the overhead
> instead. Which is statistically a much larger number of people. If we
> are going to be unfair, we might as well go for the most simple
> solution.
> 
> So I'm sorry but I can't see how your approach is better than just
> ifdef'd platform-specific code in the device driver, as we have been
> doing so far.
> 
> > I agree that tying this up with I2C bus notifier might not be the best
> > solution overall (it was simply available tight at this time) and we
> > could look into hooking this up into ACPI device enumeration to supply
> > missing properties.
> 
> I have a hard time understanding how you did not come up with this idea
> first, to be honest. If the problem you need to solve is caused by
> missing data in the ACPI tables, injecting the missing data at the ACPI
> level would seem to be the most straightforward way to go. Fixing the
> problem as close as possible to its source. But I will admit I am not
> familiar enough with the ACPI subsystem implementation to say if that
> can be done today or if preparatory work is needed.
> 
> > > PS: I hope this code will die, but if it stays, I see that you call
> > > to_i2c_client() on the notified device without checking if it is an
> > > i2c_client. It could be an i2c_adapter instead, and then you are
> > > forcibly mapping a struct i2c_client on top of a struct i2c_adapter.
> > > Good luck when you will access client->name a few lines later...
> > > i2c_verify_client() is what you want to use instead of to_i2c_client()
> > > in that situtation.
> > 
> > That is also should be fixed in -next. Because i2c_adapter structure is
> > big enough so that looking up client->name in adapter is not going to
> > blow up, nor will it likely to match, it was decided it could stay till
> > next merge window.
> 
> I'm speechless. If you believe that this kind of bug isn't worth fixing
> quickly

Would you mind telling me what is s horrible about this bug that
requires it to be fixed immediately. In practical terms please.

>, I'll go disable that piece of cra^H^Hode from the opeSUSE
> kernel for the time being. And then, go figure if/when anyone will
> remember to re-enable it once it is in an acceptable state.

Well, if you want to get in a tizzy over this - its your kernel.

> 
> I thought the rule about upstream kernel code acceptance was that you
> should get things right first, no matter how much time and how many
> iterations it takes, and then we commit the result. Apparently this has
> changed while I was not looking :-(

Yes, that is surely the rule. We do not merge the code until it is
absolutely, 100% bug free. That is why are about to release 4.11 and
stable 4.9 is at 25.

Look, it was missed, it was fixed, the bug was judged not critical
enough to drop everything and merge immediately. If you show how the bug
can affect real users I am sure Darren will expedite its merging.

Thanks.

-- 
Dmitry

Re: Silead DMI driver

[Jean Delvare]

Hi Dmirty,

On Fri, 28 Apr 2017 10:25:29 -0700, Dmitry Torokhov wrote:
> On Fri, Apr 28, 2017 at 11:33:21AM +0200, Jean Delvare wrote:
> > While this is a laudable goal, how will it scale in practice? I mean,
> > most drivers do need quirks in one form or another. Are we really going
> > to double the number of "drivers" (as in .c files and modules) and
> > Kconfig options to cleanly separate quirks from the device drivers
> > proper?
> 
> I do not see minimizing number of drivers as a goal. I also expect that

I do. A number of operations are directly dependent of the number of
available or loaded drivers. Splitting things into more small pieces
only helps performance up to a point, after which it backfires.

> devices that get one driver wrong would require more fixups. As far as

Not sure what you mean, sorry. "Getting one driver wrong"?

> having multiple config options - you do realize that even if we move
> these chunks into existing drivers they can still be protected by config
> options?

Good point, I indeed missed that. But somehow I think I find it less
confusing to have the driver-specific quirks option pop up right after
I say y or m to the driver in question, than having 2 seemingly
independent options in different menus in the configuration tree.
Darren just added the missing dependency, which makes things a bit
better, but doesn't solve the other "problems".

> > (...)
> > I'm speechless. If you believe that this kind of bug isn't worth
> > fixing quickly
> 
> Would you mind telling me what is s horrible about this bug that
> requires it to be fixed immediately. In practical terms please.

I guess this is just a case of the straw breaking the camel's back. The
non-modularity, then the horrible design, then the DMI check on every
I2C device addition on X86 every system, and now casting to the wrong
struct (and spending time verifying that it should be just OK in
practice, instead of applying a two-liner, obviously safe fix)... That
was just too much.

Ironically, the only reason why this (otherwise major) bug won't cause
any problem in practice is because you use strncmp to compare the names,
when strcmp would have been sufficient and preferable. So it's a case
of two mistakes compensating each other.

> > (...)
> > I thought the rule about upstream kernel code acceptance was that
> > you should get things right first, no matter how much time and how
> > many iterations it takes, and then we commit the result. Apparently
> > this has changed while I was not looking :-(
> 
> Yes, that is surely the rule. We do not merge the code until it is
> absolutely, 100% bug free. That is why are about to release 4.11 and
> stable 4.9 is at 25.

I perceive some sarcasm here ;-) But I'm not sure where you are getting
at. Of course we release imperfect code, and that's why we have the
stable kernel branches. But the idea of these branches is to backport
fixes that were found after the branch was released.

In this case, I see the 2 fixes have been written on April 3rd and
committed on April 13th, which correspond to rc1 and rc2 of the v4.11
release, respectively. It means you had plenty of time to send these
fixes to Linus before v4.11 final. You decided not to. This has nothing
to do with the stable branches process.

I remember from my classes at school about software engineering: "the
sooner a problem is detected and fixed, the lower the cost of the fix."
It seems that was have a good case for a study here. Just think of the
time we all have spent writing and reading these mails (and I deleted
half of what I wrote before sending it.)

-- 
Jean Delvare
SUSE L3 Support

Re: Silead DMI driver

[Darren Hart]

On Wed, May 03, 2017 at 10:19:45AM +0200, Jean Delvare wrote:
> > 
> > Yes, that is surely the rule. We do not merge the code until it is
> > absolutely, 100% bug free. That is why are about to release 4.11 and
> > stable 4.9 is at 25.
> 
> I perceive some sarcasm here ;-) But I'm not sure where you are getting
> at. Of course we release imperfect code, and that's why we have the
> stable kernel branches. But the idea of these branches is to backport
> fixes that were found after the branch was released.
> 
> In this case, I see the 2 fixes have been written on April 3rd and
> committed on April 13th, which correspond to rc1 and rc2 of the v4.11
> release, respectively. It means you had plenty of time to send these
> fixes to Linus before v4.11 final. You decided not to. This has nothing
> to do with the stable branches process.

The determination for what gets sent to Linus at what point in the rc cycle is
*really* subjective and involves a number of human factors. e.g. If it would
have been OK for RC2, but the maintainer was sick that week and Linus was
trigger happy, then by RC4 it might no longer be deemed acceptable change. It
just isn't as black and white as we'd sometimes like processes to be. Things
like this are also prioritized relative the other patches and fixes in the queue
at the time, which affects where available time can be spent.

Perhaps we made the wrong call here, that's entirely possible, and I'm perfectly
up to accepting that. It's also possible if you had all the context from that
time (which cannot be embodied entirely in the email thread) you would have made
the same call. So, points well taken, and I will certainly be more aware of
these sorts of issues going forward.

-- 
Darren Hart
VMware Open Source Technology Center