Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-04-03 09:59 keltezéssel, Boszormenyi Zoltan írta:

Hi,

2017-04-03 08:34 keltezéssel, Paul Menzel írta:

Dear Zoltán,


Am Samstag, den 01.04.2017, 12:13 +0200 schrieb Boszormenyi Zoltan:

[…]


and have split the patch into three pieces now (USB quirks, i2c-piix4
and sp5100_tco) and they were sent to the relevant mailing lists.


Could you please add me to the receiver list of these patches, so that
I can test them? Maybe also Christian (the commit author introducing
the regression), Tim (bug reporter), and Nehal from AMD?

If you uploaded them to the Kernel.org Bugtracker, that’d also work for
me.


did both.


There's a new set of packages sent to all relevant mailing lists and
also attached to the kernel bugtracker at
https://bugzilla.kernel.org/show_bug.cgi?id=170741

Hopefully this approach will work for everyone.



Best regards,
Zoltán Böszörményi




Thanks,

Paul

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-04-03 09:59 keltezéssel, Boszormenyi Zoltan írta:

Hi,

2017-04-03 08:34 keltezéssel, Paul Menzel írta:

Dear Zoltán,


Am Samstag, den 01.04.2017, 12:13 +0200 schrieb Boszormenyi Zoltan:

[…]


and have split the patch into three pieces now (USB quirks, i2c-piix4
and sp5100_tco) and they were sent to the relevant mailing lists.


Could you please add me to the receiver list of these patches, so that
I can test them? Maybe also Christian (the commit author introducing
the regression), Tim (bug reporter), and Nehal from AMD?

If you uploaded them to the Kernel.org Bugtracker, that’d also work for
me.


did both.


There's a new set of packages sent to all relevant mailing lists and
also attached to the kernel bugtracker at
https://bugzilla.kernel.org/show_bug.cgi?id=170741

Hopefully this approach will work for everyone.



Best regards,
Zoltán Böszörményi




Thanks,

Paul

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

Hi,

2017-04-03 08:34 keltezéssel, Paul Menzel írta:

Dear Zoltán,


Am Samstag, den 01.04.2017, 12:13 +0200 schrieb Boszormenyi Zoltan:

[…]


and have split the patch into three pieces now (USB quirks, i2c-piix4
and sp5100_tco) and they were sent to the relevant mailing lists.


Could you please add me to the receiver list of these patches, so that
I can test them? Maybe also Christian (the commit author introducing
the regression), Tim (bug reporter), and Nehal from AMD?

If you uploaded them to the Kernel.org Bugtracker, that’d also work for
me.


did both.

Best regards,
Zoltán Böszörményi




Thanks,

Paul

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

Hi,

2017-04-03 08:34 keltezéssel, Paul Menzel írta:

Dear Zoltán,


Am Samstag, den 01.04.2017, 12:13 +0200 schrieb Boszormenyi Zoltan:

[…]


and have split the patch into three pieces now (USB quirks, i2c-piix4
and sp5100_tco) and they were sent to the relevant mailing lists.


Could you please add me to the receiver list of these patches, so that
I can test them? Maybe also Christian (the commit author introducing
the regression), Tim (bug reporter), and Nehal from AMD?

If you uploaded them to the Kernel.org Bugtracker, that’d also work for
me.


did both.

Best regards,
Zoltán Böszörményi




Thanks,

Paul

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Paul Menzel]

Dear Zoltán,


Am Samstag, den 01.04.2017, 12:13 +0200 schrieb Boszormenyi Zoltan:

[…]

> and have split the patch into three pieces now (USB quirks, i2c-piix4 
> and sp5100_tco) and they were sent to the relevant mailing lists.

Could you please add me to the receiver list of these patches, so that
I can test them? Maybe also Christian (the commit author introducing
the regression), Tim (bug reporter), and Nehal from AMD?

If you uploaded them to the Kernel.org Bugtracker, that’d also work for
me.


Thanks,

Paul

signature.asc
Description: This is a digitally signed message part

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Paul Menzel]

Dear Zoltán,


Am Samstag, den 01.04.2017, 12:13 +0200 schrieb Boszormenyi Zoltan:

[…]

> and have split the patch into three pieces now (USB quirks, i2c-piix4 
> and sp5100_tco) and they were sent to the relevant mailing lists.

Could you please add me to the receiver list of these patches, so that
I can test them? Maybe also Christian (the commit author introducing
the regression), Tim (bug reporter), and Nehal from AMD?

If you uploaded them to the Kernel.org Bugtracker, that’d also work for
me.


Thanks,

Paul

signature.asc
Description: This is a digitally signed message part

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-04-01 18:20 keltezéssel, Boszormenyi Zoltan írta:

The best clean alternative would be add new resource handling infrastructure.
* Expose the currently static alloc_resource() in kernel/resource.c
  With this, driver initialization can allocate the resource once
  for the lifetime of the driver and it it fails,


(unfinished sentence) then the failure is during driver initialization,
not during runtime, possibly days or weeks later.


* Add a new insert_muxed_region() / __insert_muxed_region() function with
  different semantics from request_muxed_region() / __request_region():
  1 Accept a pointer to already allocated resource.
  2 If the conflicting resource doesn't have IORESOURCE_MUXED set,
complain loudly in the syslog but still go into the wait queue.
The conflicting resource also has the name which can be printed
so the inconsistent resource / region usage can be fixed.
  We can also just modify the __request_region() semantics, so:
  1 It accepts a pointer to an allocated resource or NULL.
In the second case, the resource is allocated internally and can
still fail.
  2 The above second point. But this may cause an error in code that
expects the old semantics.

The window for request_muxed_region()+release_region() is so short
that the requested I/O port range would not show up in /proc/ioports.

All this would be to fix only 3 drivers in a no-error scenario and only
achieving the functionality of a mutex seems to be overkill.

Another alternative is to revert commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
that caused the regression in sp5100_tco in the first place.

Best regards,
Zoltán Böszörményi

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-04-01 18:20 keltezéssel, Boszormenyi Zoltan írta:

The best clean alternative would be add new resource handling infrastructure.
* Expose the currently static alloc_resource() in kernel/resource.c
  With this, driver initialization can allocate the resource once
  for the lifetime of the driver and it it fails,


(unfinished sentence) then the failure is during driver initialization,
not during runtime, possibly days or weeks later.


* Add a new insert_muxed_region() / __insert_muxed_region() function with
  different semantics from request_muxed_region() / __request_region():
  1 Accept a pointer to already allocated resource.
  2 If the conflicting resource doesn't have IORESOURCE_MUXED set,
complain loudly in the syslog but still go into the wait queue.
The conflicting resource also has the name which can be printed
so the inconsistent resource / region usage can be fixed.
  We can also just modify the __request_region() semantics, so:
  1 It accepts a pointer to an allocated resource or NULL.
In the second case, the resource is allocated internally and can
still fail.
  2 The above second point. But this may cause an error in code that
expects the old semantics.

The window for request_muxed_region()+release_region() is so short
that the requested I/O port range would not show up in /proc/ioports.

All this would be to fix only 3 drivers in a no-error scenario and only
achieving the functionality of a mutex seems to be overkill.

Another alternative is to revert commit 2fee61d22e606fc99ade9079fda15fdee83ec33e
that caused the regression in sp5100_tco in the first place.

Best regards,
Zoltán Böszörményi

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Guenter Roeck]

On 04/01/2017 03:13 AM, Boszormenyi Zoltan wrote:

2017-03-31 17:05 keltezéssel, Guenter Roeck írta:

On Fri, Mar 31, 2017 at 04:46:02PM +0200, Boszormenyi Zoltan wrote:

2017-03-31 14:49 keltezéssel, Guenter Roeck írta:

request_muxed_region() can fail, and literally every other driver
using it checks for that failure. Please do the same.


In what circumstances can request_muxed_region() fail? As far as
I can see, only if two drivers use the same I/O port base and the
already present region did not use IORESOURCE_MUXED which is
not the case here. When request_muxed_region() is used consistently,
subsequent requests are put on a wait queue and the first one is
woken up when the region is released. So, it's basically a mutex.
Am I missing something here?



Yes. failure to allocate the resource is one.


So, a common mutex should be used.



Just because you don't want to check for errors ?

I am not on favor of your new solution. I think it violates layering all over
the place, and I dislike the idea of having a global mutex as you propose.
I won't shut it down, but I'll let others provide feedback on your new series
of patches.

Guenter


I have also added synchronization to the USB PCI quirks code and
have split the patch into three pieces now (USB quirks, i2c-piix4 and
sp5100_tco) and they were sent to the relevant mailing lists.

I don't know which subsystem wants to take it, all 3 patches are
needed at once.

Best regards,
Zoltán Böszörményi

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Guenter Roeck]

On 04/01/2017 03:13 AM, Boszormenyi Zoltan wrote:

2017-03-31 17:05 keltezéssel, Guenter Roeck írta:

On Fri, Mar 31, 2017 at 04:46:02PM +0200, Boszormenyi Zoltan wrote:

2017-03-31 14:49 keltezéssel, Guenter Roeck írta:

request_muxed_region() can fail, and literally every other driver
using it checks for that failure. Please do the same.


In what circumstances can request_muxed_region() fail? As far as
I can see, only if two drivers use the same I/O port base and the
already present region did not use IORESOURCE_MUXED which is
not the case here. When request_muxed_region() is used consistently,
subsequent requests are put on a wait queue and the first one is
woken up when the region is released. So, it's basically a mutex.
Am I missing something here?



Yes. failure to allocate the resource is one.


So, a common mutex should be used.



Just because you don't want to check for errors ?

I am not on favor of your new solution. I think it violates layering all over
the place, and I dislike the idea of having a global mutex as you propose.
I won't shut it down, but I'll let others provide feedback on your new series
of patches.

Guenter


I have also added synchronization to the USB PCI quirks code and
have split the patch into three pieces now (USB quirks, i2c-piix4 and
sp5100_tco) and they were sent to the relevant mailing lists.

I don't know which subsystem wants to take it, all 3 patches are
needed at once.

Best regards,
Zoltán Böszörményi

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-03-31 17:05 keltezéssel, Guenter Roeck írta:

On Fri, Mar 31, 2017 at 04:46:02PM +0200, Boszormenyi Zoltan wrote:

2017-03-31 14:49 keltezéssel, Guenter Roeck írta:

request_muxed_region() can fail, and literally every other driver
using it checks for that failure. Please do the same.


In what circumstances can request_muxed_region() fail? As far as
I can see, only if two drivers use the same I/O port base and the
already present region did not use IORESOURCE_MUXED which is
not the case here. When request_muxed_region() is used consistently,
subsequent requests are put on a wait queue and the first one is
woken up when the region is released. So, it's basically a mutex.
Am I missing something here?



Yes. failure to allocate the resource is one.


So, a common mutex should be used.

I have also added synchronization to the USB PCI quirks code and
have split the patch into three pieces now (USB quirks, i2c-piix4 and
sp5100_tco) and they were sent to the relevant mailing lists.

I don't know which subsystem wants to take it, all 3 patches are
needed at once.

Best regards,
Zoltán Böszörményi

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Boszormenyi Zoltan]

2017-03-31 17:05 keltezéssel, Guenter Roeck írta:

On Fri, Mar 31, 2017 at 04:46:02PM +0200, Boszormenyi Zoltan wrote:

2017-03-31 14:49 keltezéssel, Guenter Roeck írta:

request_muxed_region() can fail, and literally every other driver
using it checks for that failure. Please do the same.


In what circumstances can request_muxed_region() fail? As far as
I can see, only if two drivers use the same I/O port base and the
already present region did not use IORESOURCE_MUXED which is
not the case here. When request_muxed_region() is used consistently,
subsequent requests are put on a wait queue and the first one is
woken up when the region is released. So, it's basically a mutex.
Am I missing something here?



Yes. failure to allocate the resource is one.


So, a common mutex should be used.

I have also added synchronization to the USB PCI quirks code and
have split the patch into three pieces now (USB quirks, i2c-piix4 and
sp5100_tco) and they were sent to the relevant mailing lists.

I don't know which subsystem wants to take it, all 3 patches are
needed at once.

Best regards,
Zoltán Böszörményi

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Guenter Roeck]

On Fri, Mar 31, 2017 at 04:46:02PM +0200, Boszormenyi Zoltan wrote:
> 2017-03-31 14:49 keltezéssel, Guenter Roeck írta:
> >Hi Paul,
> >
> >On 03/31/2017 12:17 AM, Paul Menzel wrote:
> >>Dear Wolfram,
> >>
> >>
> >>Thank you for the reply, which we talked about briefly at the
> >>Chemnitzer LinuxTage.
> >>
> >>
> >>Am Freitag, den 03.03.2017, 11:17 +0100 schrieb Wolfram Sang:
> Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
> multiplexed main adapter in SB800) [1] caused a regression. Tim
> reported that to the Linux Kernel Bugtracker as bug #170741 last
> September [2], but it looks like the affected subsystems don’t use it.
> >>>
> >>>Jean Delvare pointed out this issue amongst others[1] last year already.
> >>>Let me quote:
> >>>
> >>>===
> >>>
> >>>5* The I/O ports used for SMBus configuration and port switching are
> >>>also needed by a watchdog driver, sp5100_tco. Both drivers request the
> >>>region, so the first one wins, and the other driver can't be loaded.
> >>>sp5100_tco was there first, so the changes done to the i2c-piix4 driver
> >>>recently will cause a regression for some users by preventing them
> >>>from using the sp5100_tco and i2c-piix4 drivers at the same time. In
> >>>the long run I guess we will need a helper module to handle this shared
> >>>resource. Unless IORESOURCE_MUXED can be used for that. Either way,
> >>>that's more work than I can put into this before kernel v4.5 is
> >>>released. For the time being, I think we should simply make it
> >>>non-fatal if the I/O ports can't be requested, and continue without
> >>>multiplexing (as before.)
> >>>
> >>>===
> >>>
> >>>Seems nobody had the resources, so far.
> >>
> >>I still don’t understand, why Jean then not immediately reverted the
> >>commit to adhere to the Linux Kernel’s no-regression-policy.
> >>
> >>>I don't have the HW and not much experience with non-embedded
> >>>platforms. I wonder, though, if we really need to convert the drivers
> >>>to MFD ones, or if we could use the simpler MFD_SYSCON mechanism
> >>>which helps in exactly such cases for embedded platforms. But I am
> >>>really lacking details here and am afraid this is probably all the
> >>>input I can give currently.
> >>
> >>Zoltan stepped up, and uploaded a patch for review to the Kernel.org
> >>Bugzilla [2], also attached to this message.
> >>
> >
> >Please don't send patches as attachments.
> >
> >request_muxed_region() can fail, and literally every other driver
> >using it checks for that failure. Please do the same.
> 
> In what circumstances can request_muxed_region() fail? As far as
> I can see, only if two drivers use the same I/O port base and the
> already present region did not use IORESOURCE_MUXED which is
> not the case here. When request_muxed_region() is used consistently,
> subsequent requests are put on a wait queue and the first one is
> woken up when the region is released. So, it's basically a mutex.
> Am I missing something here?
> 

Yes. failure to allocate the resource is one. The other is that you are
making the assumption that all other requesters have IORESOURCE_MUXED set.
There is no guarantee that this is the case.

Guenter

> Alternatively, the original "piix4_mutex_sb800" mutex can be moved out
> to its own file and used by both drivers. This way, request_muxed_region()
> would not be needed at all among these two drivers.
> 
> The third option is to remove the request_*region() calls and the mutex
> completely.
> 
> After all, drivers/usb/host/pci-quirks.c::usb_amd_quirk_pll() also uses
> this I/O port pair and this is done without request_*region() or locking.
> It is for some AMD devices, SB800 included, for which the function
> accesses the same I/O ports used by both i2c-piix4 and sp5100_tco.
> 
> /*
>  * The hardware normally enables the A-link power management feature, which
>  * lets the system lower the power consumption in idle states.
>  *
>  * This USB quirk prevents the link going into that lower power state
>  * during isochronous transfers.
>  *
>  * Without this quirk, isochronous stream on OHCI/EHCI/xHCI controllers of
>  * some AMD platforms may stutter or have breaks occasionally.
>  */
> static void usb_amd_quirk_pll(int disable)
> 
> This function is hidden behind two wrappers: usb_amd_quirk_pll_disable()
> and usb_amd_quirk_pll_enable() and these are used from:
> 
> drivers/usb/host/ohci-q.c
> drivers/usb/host/xhci-ring.c
> drivers/usb/host/ehci-sched.c
> 
> >The sp5100_tco_dev_name change in the watchdog driver is unnecessary.
> 
> request_muxed_region() requires a const char * name to be passed.
> sp5100_tco uses a different DEVNAME for the two device generations.
> I wanted to preserve this distinction but dev_name is local to
> sp5100_tco_setupdevice() and it would have been an overkill to call
> tco_has_sp5100_reg_layout() every time the code executes
> request_muxed_region().
> 
> Are you saying that this distinction is unnecessary, too?
> I would prefer a single DEVNAME 

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Guenter Roeck]

On Fri, Mar 31, 2017 at 04:46:02PM +0200, Boszormenyi Zoltan wrote:
> 2017-03-31 14:49 keltezéssel, Guenter Roeck írta:
> >Hi Paul,
> >
> >On 03/31/2017 12:17 AM, Paul Menzel wrote:
> >>Dear Wolfram,
> >>
> >>
> >>Thank you for the reply, which we talked about briefly at the
> >>Chemnitzer LinuxTage.
> >>
> >>
> >>Am Freitag, den 03.03.2017, 11:17 +0100 schrieb Wolfram Sang:
> Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
> multiplexed main adapter in SB800) [1] caused a regression. Tim
> reported that to the Linux Kernel Bugtracker as bug #170741 last
> September [2], but it looks like the affected subsystems don’t use it.
> >>>
> >>>Jean Delvare pointed out this issue amongst others[1] last year already.
> >>>Let me quote:
> >>>
> >>>===
> >>>
> >>>5* The I/O ports used for SMBus configuration and port switching are
> >>>also needed by a watchdog driver, sp5100_tco. Both drivers request the
> >>>region, so the first one wins, and the other driver can't be loaded.
> >>>sp5100_tco was there first, so the changes done to the i2c-piix4 driver
> >>>recently will cause a regression for some users by preventing them
> >>>from using the sp5100_tco and i2c-piix4 drivers at the same time. In
> >>>the long run I guess we will need a helper module to handle this shared
> >>>resource. Unless IORESOURCE_MUXED can be used for that. Either way,
> >>>that's more work than I can put into this before kernel v4.5 is
> >>>released. For the time being, I think we should simply make it
> >>>non-fatal if the I/O ports can't be requested, and continue without
> >>>multiplexing (as before.)
> >>>
> >>>===
> >>>
> >>>Seems nobody had the resources, so far.
> >>
> >>I still don’t understand, why Jean then not immediately reverted the
> >>commit to adhere to the Linux Kernel’s no-regression-policy.
> >>
> >>>I don't have the HW and not much experience with non-embedded
> >>>platforms. I wonder, though, if we really need to convert the drivers
> >>>to MFD ones, or if we could use the simpler MFD_SYSCON mechanism
> >>>which helps in exactly such cases for embedded platforms. But I am
> >>>really lacking details here and am afraid this is probably all the
> >>>input I can give currently.
> >>
> >>Zoltan stepped up, and uploaded a patch for review to the Kernel.org
> >>Bugzilla [2], also attached to this message.
> >>
> >
> >Please don't send patches as attachments.
> >
> >request_muxed_region() can fail, and literally every other driver
> >using it checks for that failure. Please do the same.
> 
> In what circumstances can request_muxed_region() fail? As far as
> I can see, only if two drivers use the same I/O port base and the
> already present region did not use IORESOURCE_MUXED which is
> not the case here. When request_muxed_region() is used consistently,
> subsequent requests are put on a wait queue and the first one is
> woken up when the region is released. So, it's basically a mutex.
> Am I missing something here?
> 

Yes. failure to allocate the resource is one. The other is that you are
making the assumption that all other requesters have IORESOURCE_MUXED set.
There is no guarantee that this is the case.

Guenter

> Alternatively, the original "piix4_mutex_sb800" mutex can be moved out
> to its own file and used by both drivers. This way, request_muxed_region()
> would not be needed at all among these two drivers.
> 
> The third option is to remove the request_*region() calls and the mutex
> completely.
> 
> After all, drivers/usb/host/pci-quirks.c::usb_amd_quirk_pll() also uses
> this I/O port pair and this is done without request_*region() or locking.
> It is for some AMD devices, SB800 included, for which the function
> accesses the same I/O ports used by both i2c-piix4 and sp5100_tco.
> 
> /*
>  * The hardware normally enables the A-link power management feature, which
>  * lets the system lower the power consumption in idle states.
>  *
>  * This USB quirk prevents the link going into that lower power state
>  * during isochronous transfers.
>  *
>  * Without this quirk, isochronous stream on OHCI/EHCI/xHCI controllers of
>  * some AMD platforms may stutter or have breaks occasionally.
>  */
> static void usb_amd_quirk_pll(int disable)
> 
> This function is hidden behind two wrappers: usb_amd_quirk_pll_disable()
> and usb_amd_quirk_pll_enable() and these are used from:
> 
> drivers/usb/host/ohci-q.c
> drivers/usb/host/xhci-ring.c
> drivers/usb/host/ehci-sched.c
> 
> >The sp5100_tco_dev_name change in the watchdog driver is unnecessary.
> 
> request_muxed_region() requires a const char * name to be passed.
> sp5100_tco uses a different DEVNAME for the two device generations.
> I wanted to preserve this distinction but dev_name is local to
> sp5100_tco_setupdevice() and it would have been an overkill to call
> tco_has_sp5100_reg_layout() every time the code executes
> request_muxed_region().
> 
> Are you saying that this distinction is unnecessary, too?
> I would prefer a single DEVNAME 

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Guenter Roeck]

Hi Paul,

On 03/31/2017 12:17 AM, Paul Menzel wrote:

Dear Wolfram,


Thank you for the reply, which we talked about briefly at the
Chemnitzer LinuxTage.


Am Freitag, den 03.03.2017, 11:17 +0100 schrieb Wolfram Sang:

Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
multiplexed main adapter in SB800) [1] caused a regression. Tim
reported that to the Linux Kernel Bugtracker as bug #170741 last
September [2], but it looks like the affected subsystems don’t use it.


Jean Delvare pointed out this issue amongst others[1] last year already.
Let me quote:

===

5* The I/O ports used for SMBus configuration and port switching are
also needed by a watchdog driver, sp5100_tco. Both drivers request the
region, so the first one wins, and the other driver can't be loaded.
sp5100_tco was there first, so the changes done to the i2c-piix4 driver
recently will cause a regression for some users by preventing them
from using the sp5100_tco and i2c-piix4 drivers at the same time. In
the long run I guess we will need a helper module to handle this shared
resource. Unless IORESOURCE_MUXED can be used for that. Either way,
that's more work than I can put into this before kernel v4.5 is
released. For the time being, I think we should simply make it
non-fatal if the I/O ports can't be requested, and continue without
multiplexing (as before.)

===

Seems nobody had the resources, so far.


I still don’t understand, why Jean then not immediately reverted the
commit to adhere to the Linux Kernel’s no-regression-policy.


I don't have the HW and not much experience with non-embedded
platforms. I wonder, though, if we really need to convert the drivers
to MFD ones, or if we could use the simpler MFD_SYSCON mechanism
which helps in exactly such cases for embedded platforms. But I am
really lacking details here and am afraid this is probably all the
input I can give currently.


Zoltan stepped up, and uploaded a patch for review to the Kernel.org
Bugzilla [2], also attached to this message.



Please don't send patches as attachments.

request_muxed_region() can fail, and literally every other driver
using it checks for that failure. Please do the same.

The sp5100_tco_dev_name change in the watchdog driver is unnecessary.
There are some unnecessary { } in the watchdog driver after the patch
is applied.

Please split the patch into two patches so they can be reviewed and
applied separately.

Thanks,
Guenter

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Guenter Roeck]

Hi Paul,

On 03/31/2017 12:17 AM, Paul Menzel wrote:

Dear Wolfram,


Thank you for the reply, which we talked about briefly at the
Chemnitzer LinuxTage.


Am Freitag, den 03.03.2017, 11:17 +0100 schrieb Wolfram Sang:

Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
multiplexed main adapter in SB800) [1] caused a regression. Tim
reported that to the Linux Kernel Bugtracker as bug #170741 last
September [2], but it looks like the affected subsystems don’t use it.


Jean Delvare pointed out this issue amongst others[1] last year already.
Let me quote:

===

5* The I/O ports used for SMBus configuration and port switching are
also needed by a watchdog driver, sp5100_tco. Both drivers request the
region, so the first one wins, and the other driver can't be loaded.
sp5100_tco was there first, so the changes done to the i2c-piix4 driver
recently will cause a regression for some users by preventing them
from using the sp5100_tco and i2c-piix4 drivers at the same time. In
the long run I guess we will need a helper module to handle this shared
resource. Unless IORESOURCE_MUXED can be used for that. Either way,
that's more work than I can put into this before kernel v4.5 is
released. For the time being, I think we should simply make it
non-fatal if the I/O ports can't be requested, and continue without
multiplexing (as before.)

===

Seems nobody had the resources, so far.


I still don’t understand, why Jean then not immediately reverted the
commit to adhere to the Linux Kernel’s no-regression-policy.


I don't have the HW and not much experience with non-embedded
platforms. I wonder, though, if we really need to convert the drivers
to MFD ones, or if we could use the simpler MFD_SYSCON mechanism
which helps in exactly such cases for embedded platforms. But I am
really lacking details here and am afraid this is probably all the
input I can give currently.


Zoltan stepped up, and uploaded a patch for review to the Kernel.org
Bugzilla [2], also attached to this message.



Please don't send patches as attachments.

request_muxed_region() can fail, and literally every other driver
using it checks for that failure. Please do the same.

The sp5100_tco_dev_name change in the watchdog driver is unnecessary.
There are some unnecessary { } in the watchdog driver after the patch
is applied.

Please split the patch into two patches so they can be reviewed and
applied separately.

Thanks,
Guenter

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Paul Menzel]

Dear Wolfram,


Thank you for the reply, which we talked about briefly at the
Chemnitzer LinuxTage.


Am Freitag, den 03.03.2017, 11:17 +0100 schrieb Wolfram Sang:
> > Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
> > multiplexed main adapter in SB800) [1] caused a regression. Tim
> > reported that to the Linux Kernel Bugtracker as bug #170741 last
> > September [2], but it looks like the affected subsystems don’t use it.
> 
> Jean Delvare pointed out this issue amongst others[1] last year already.
> Let me quote:
> 
> ===
> 
> 5* The I/O ports used for SMBus configuration and port switching are
> also needed by a watchdog driver, sp5100_tco. Both drivers request the
> region, so the first one wins, and the other driver can't be loaded.
> sp5100_tco was there first, so the changes done to the i2c-piix4 driver
> recently will cause a regression for some users by preventing them
> from using the sp5100_tco and i2c-piix4 drivers at the same time. In
> the long run I guess we will need a helper module to handle this shared
> resource. Unless IORESOURCE_MUXED can be used for that. Either way,
> that's more work than I can put into this before kernel v4.5 is
> released. For the time being, I think we should simply make it
> non-fatal if the I/O ports can't be requested, and continue without
> multiplexing (as before.)
> 
> ===
> 
> Seems nobody had the resources, so far.

I still don’t understand, why Jean then not immediately reverted the
commit to adhere to the Linux Kernel’s no-regression-policy.

> I don't have the HW and not much experience with non-embedded
> platforms. I wonder, though, if we really need to convert the drivers
> to MFD ones, or if we could use the simpler MFD_SYSCON mechanism
> which helps in exactly such cases for embedded platforms. But I am
> really lacking details here and am afraid this is probably all the
> input I can give currently.

Zoltan stepped up, and uploaded a patch for review to the Kernel.org
Bugzilla [2], also attached to this message.

Christian, Tim, and Nehal could you please test and review it?


Thanks,

Paul


> [1] http://www.spinics.net/lists/linux-i2c/msg23437.html
[2] https://bugzilla.kernel.org/show_bug.cgi?id=170741From: Zoltán Böszörményi 
Date: Tue Mar 28 14:53:07 2017 +0200
Subject: [PATCH] watchdog/sp5100_tco: Coexist with i2c-piix

Currently, the kernel says this when i2c-piix loads before sp5100_tco:

sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
sp5100_tco: I/O address 0x0cd6 already in use

Both i2c-piix4 and sp5100_tco uses a static request_region() call
so it depends on the load order which one wins.

i2c-piix4 uses a mutex to protect I/O port accesses to the pair of
I/O ports. Replace this mutex lock / unlock with request_muxed_region()
and release_region() around the I/O port accesses in i2c-piix4.

Add request_muxed_region() / release_region() pairs around the I/O
accesses in sp5100_tco.

This will act as a cross-driver mutex.

Signed-off-by: Zoltán Böszörményi 

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index c21ca7b..16befdd5 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -40,7 +40,6 @@
 #include 
 #include 
 #include 
-#include 
 
 
 /* PIIX4 SMBus address offsets */
@@ -144,10 +143,9 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 
 /*
  * SB800 globals
- * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
- * of I/O ports at SB800_PIIX4_SMB_IDX.
+ * the pair of I/O ports at SB800_PIIX4_SMB_IDX are protected
+ * by request_muxed_region / release_region
  */
-static DEFINE_MUTEX(piix4_mutex_sb800);
 static u8 piix4_port_sel_sb800;
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	" port 0", " port 2", " port 3", " port 4"
@@ -157,8 +155,6 @@ static const char *piix4_aux_port_name_sb800 = " port 1";
 struct i2c_piix4_adapdata {
 	unsigned short smba;
 
-	/* SB800 */
-	bool sb800_main;
 	u8 port;		/* Port number, shifted */
 };
 
@@ -261,6 +257,14 @@ static int piix4_setup(struct pci_dev *PIIX4_dev,
 	return piix4_smba;
 }
 
+static inline void enter_sb800(void) {
+	request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx");
+}
+
+static inline void leave_sb800(void) {
+	release_region(SB800_PIIX4_SMB_IDX, 2);
+}
+
 static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 			 const struct pci_device_id *id, u8 aux)
 {
@@ -286,12 +290,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	mutex_lock(_mutex_sb800);
+	enter_sb800();
 	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
 	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
-	mutex_unlock(_mutex_sb800);
+	leave_sb800();
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -349,13 +353,13 @@ static int 

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Paul Menzel]

Dear Wolfram,


Thank you for the reply, which we talked about briefly at the
Chemnitzer LinuxTage.


Am Freitag, den 03.03.2017, 11:17 +0100 schrieb Wolfram Sang:
> > Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
> > multiplexed main adapter in SB800) [1] caused a regression. Tim
> > reported that to the Linux Kernel Bugtracker as bug #170741 last
> > September [2], but it looks like the affected subsystems don’t use it.
> 
> Jean Delvare pointed out this issue amongst others[1] last year already.
> Let me quote:
> 
> ===
> 
> 5* The I/O ports used for SMBus configuration and port switching are
> also needed by a watchdog driver, sp5100_tco. Both drivers request the
> region, so the first one wins, and the other driver can't be loaded.
> sp5100_tco was there first, so the changes done to the i2c-piix4 driver
> recently will cause a regression for some users by preventing them
> from using the sp5100_tco and i2c-piix4 drivers at the same time. In
> the long run I guess we will need a helper module to handle this shared
> resource. Unless IORESOURCE_MUXED can be used for that. Either way,
> that's more work than I can put into this before kernel v4.5 is
> released. For the time being, I think we should simply make it
> non-fatal if the I/O ports can't be requested, and continue without
> multiplexing (as before.)
> 
> ===
> 
> Seems nobody had the resources, so far.

I still don’t understand, why Jean then not immediately reverted the
commit to adhere to the Linux Kernel’s no-regression-policy.

> I don't have the HW and not much experience with non-embedded
> platforms. I wonder, though, if we really need to convert the drivers
> to MFD ones, or if we could use the simpler MFD_SYSCON mechanism
> which helps in exactly such cases for embedded platforms. But I am
> really lacking details here and am afraid this is probably all the
> input I can give currently.

Zoltan stepped up, and uploaded a patch for review to the Kernel.org
Bugzilla [2], also attached to this message.

Christian, Tim, and Nehal could you please test and review it?


Thanks,

Paul


> [1] http://www.spinics.net/lists/linux-i2c/msg23437.html
[2] https://bugzilla.kernel.org/show_bug.cgi?id=170741From: Zoltán Böszörményi 
Date: Tue Mar 28 14:53:07 2017 +0200
Subject: [PATCH] watchdog/sp5100_tco: Coexist with i2c-piix

Currently, the kernel says this when i2c-piix loads before sp5100_tco:

sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver v0.05
sp5100_tco: PCI Vendor ID: 0x1002, Device ID: 0x4385, Revision ID: 0x42
sp5100_tco: I/O address 0x0cd6 already in use

Both i2c-piix4 and sp5100_tco uses a static request_region() call
so it depends on the load order which one wins.

i2c-piix4 uses a mutex to protect I/O port accesses to the pair of
I/O ports. Replace this mutex lock / unlock with request_muxed_region()
and release_region() around the I/O port accesses in i2c-piix4.

Add request_muxed_region() / release_region() pairs around the I/O
accesses in sp5100_tco.

This will act as a cross-driver mutex.

Signed-off-by: Zoltán Böszörményi 

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index c21ca7b..16befdd5 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -40,7 +40,6 @@
 #include 
 #include 
 #include 
-#include 
 
 
 /* PIIX4 SMBus address offsets */
@@ -144,10 +143,9 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
 
 /*
  * SB800 globals
- * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
- * of I/O ports at SB800_PIIX4_SMB_IDX.
+ * the pair of I/O ports at SB800_PIIX4_SMB_IDX are protected
+ * by request_muxed_region / release_region
  */
-static DEFINE_MUTEX(piix4_mutex_sb800);
 static u8 piix4_port_sel_sb800;
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	" port 0", " port 2", " port 3", " port 4"
@@ -157,8 +155,6 @@ static const char *piix4_aux_port_name_sb800 = " port 1";
 struct i2c_piix4_adapdata {
 	unsigned short smba;
 
-	/* SB800 */
-	bool sb800_main;
 	u8 port;		/* Port number, shifted */
 };
 
@@ -261,6 +257,14 @@ static int piix4_setup(struct pci_dev *PIIX4_dev,
 	return piix4_smba;
 }
 
+static inline void enter_sb800(void) {
+	request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx");
+}
+
+static inline void leave_sb800(void) {
+	release_region(SB800_PIIX4_SMB_IDX, 2);
+}
+
 static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 			 const struct pci_device_id *id, u8 aux)
 {
@@ -286,12 +290,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	mutex_lock(_mutex_sb800);
+	enter_sb800();
 	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
 	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
-	mutex_unlock(_mutex_sb800);
+	leave_sb800();
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -349,13 +353,13 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Wolfram Sang]

> Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
> multiplexed main adapter in SB800) [1] caused a regression. Tim
> reported that to the Linux Kernel Bugtracker as bug #170741 last
> September [2], but it looks like the affected subsystems don’t use it.

Jean Delvare pointed out this issue amongst others[1] last year already.
Let me quote:

===

5* The I/O ports used for SMBus configuration and port switching are
also needed by a watchdog driver, sp5100_tco. Both drivers request the
region, so the first one wins, and the other driver can't be loaded.
sp5100_tco was there first, so the changes done to the i2c-piix4 driver
recently will cause a regression for some users by preventing them
from using the sp5100_tco and i2c-piix4 drivers at the same time. In
the long run I guess we will need a helper module to handle this shared
resource. Unless IORESOURCE_MUXED can be used for that. Either way,
that's more work than I can put into this before kernel v4.5 is
released. For the time being, I think we should simply make it
non-fatal if the I/O ports can't be requested, and continue without
multiplexing (as before.)

===

Seems nobody had the resources, so far. I don't have the HW and not much
experience with non-embedded platforms. I wonder, though, if we really
need to convert the drivers to MFD ones, or if we could use the simpler
MFD_SYSCON mechanism which helps in exactly such cases for embedded
platforms. But I am really lacking details here and am afraid this is
probably all the input I can give currently.

Regards,

   Wolfram

[1] http://www.spinics.net/lists/linux-i2c/msg23437.html



signature.asc
Description: PGP signature

Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

[Wolfram Sang]

> Unfortunately, commit 2fee61d22e (i2c: piix4: Add support for
> multiplexed main adapter in SB800) [1] caused a regression. Tim
> reported that to the Linux Kernel Bugtracker as bug #170741 last
> September [2], but it looks like the affected subsystems don’t use it.

Jean Delvare pointed out this issue amongst others[1] last year already.
Let me quote:

===

5* The I/O ports used for SMBus configuration and port switching are
also needed by a watchdog driver, sp5100_tco. Both drivers request the
region, so the first one wins, and the other driver can't be loaded.
sp5100_tco was there first, so the changes done to the i2c-piix4 driver
recently will cause a regression for some users by preventing them
from using the sp5100_tco and i2c-piix4 drivers at the same time. In
the long run I guess we will need a helper module to handle this shared
resource. Unless IORESOURCE_MUXED can be used for that. Either way,
that's more work than I can put into this before kernel v4.5 is
released. For the time being, I think we should simply make it
non-fatal if the I/O ports can't be requested, and continue without
multiplexing (as before.)

===

Seems nobody had the resources, so far. I don't have the HW and not much
experience with non-embedded platforms. I wonder, though, if we really
need to convert the drivers to MFD ones, or if we could use the simpler
MFD_SYSCON mechanism which helps in exactly such cases for embedded
platforms. But I am really lacking details here and am afraid this is
probably all the input I can give currently.

Regards,

   Wolfram

[1] http://www.spinics.net/lists/linux-i2c/msg23437.html



signature.asc
Description: PGP signature