Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset

On the AMD Turion N40L and other related SoCs, the i2c-piix4 driver now claims the 0xcd6 ioport, preventing the sp5100_tco watchdog driver from loading:

piix4_smbus 0000:00:14.0: SMBus Host Controller at 0xb00, revision 0 piix4_smbus 0000:00:14.0: Using register 0x2c for SMBus port selection piix4_smbus 0000:00:14.0: Auxiliary SMBus Host Controller at 0xb20
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

This breaks watchdog operation on existing systems on upgrade and new deployments unless the i2c-piix4 driver is blacklisted.

See:

drivers/watchdog/sp5100_tco.c

tco_timer_enable(void)

(SB800_IO_PM_INDEX_REG is defined in drivers/watchdog/sp5100_tco.h)

This is the commit which prevents the watchdog driver from loading:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2fee61d22e606fc99ade9079fda15fdee83ec33e

See also AMD docs

45483_sb800_bdg_pub_3.03

Perhaps a fix is to switch both drivers from static to dynamic allocation of the IO ports in question, since the watchdog driver only accesses the port during initialisation (with backoff/retry maybe to avoid races?).

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.

Quoted 4 lines

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.

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

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:

Quoted 23 lines

Quoted 4 lines

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.)

Quoted 3 lines

===

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.

Quoted 6 lines

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.

Hi Paul,

Quoted 45 lines · 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:

Quoted 23 lines

Quoted 4 lines

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.)

Quoted 3 lines

===

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

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

Quoted 48 lines

Hi Paul,

Quoted 45 lines · 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:

Quoted 23 lines

Quoted 4 lines

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.)

Quoted 3 lines

===

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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 3 lines

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?

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

Quoted 1 line

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 that just contains the driver name. Less code, less confusion.

Quoted 2 lines

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

True, there are two places.

Quoted 2 lines

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

Likely I will, but I would like to hear the others' opinion, too.

1. a single patch with a single goal: protect I/O port access for SB800 across the whole kernel
2. patch series for same goal for the two drivers separately (or three if pci-quirks.c needs to change as well)

and

A) a common mutex across the two (three) drivers in the kernel, or
B) request_muxed_region() with retrying in the usual manner if it fails:

   #define enter_sb800() \
      do { \
      } while (!request_muxed_region(...))

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

Quoted 4 lines

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

Quoted 2 lines

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

Quoted 8 lines

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.

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

Quoted 14 lines · Boszormenyi Zoltan

Quoted 12 lines

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

Quoted 2 lines

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?

Quoted 1 line

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

Quoted 15 lines

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

Quoted 14 lines · Boszormenyi Zoltan

Quoted 12 lines

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

Quoted 2 lines

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?

Quoted 4 lines

Quoted 1 line

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.

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,

* 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

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.

Dear Zoltán,

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

[…]

Quoted 2 lines

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

Hi,

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

Quoted 10 lines

Dear Zoltán,

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

[…]

Quoted 2 lines

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.

Quoted 6 lines

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

Quoted 6 lines

Thanks,

Paul