Dear Linux folks,
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.
So I just copy his report in here, and put all the people in CC
mentioned in the commit, the bug report, and in the subsystems
I2C/SMBUS CONTROLLER DRIVERS FOR PC and WATCHDOG DEVICE DRIVERS
mentioned in the file `MAINTAINERS`.
> 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?).
Is there somebody having the resources to implement the dynamic
allocation to solve this regression?
Thanks,
Paul
[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2fee61d22e606fc99ade9079fda15fdee83ec33e
[2] https://bugzilla.kernel.org/show_bug.cgi?id=170741
> 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
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=170741
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
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 that just contains the driver name.
> Less code, less confusion.
>
> >There are some unnecessary { } in the watchdog driver after the patch
> >is applied.
>
> True, there are two places.
>
> >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
>
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
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
>
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
>
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
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
>
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
>>
>