Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.kernel > #57400

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

Cross-posted to 3 groups.

Show all headers | View raw

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
> 

Back to linux.debian.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Bug#853122: sp5100_tco: I/O address 0x0cd6 already in use Paul Menzel <pm.debian@googlemail.com> - 2017-01-29 23:30 +0100
  Processed: sp5100_tco: I/O address 0x0cd6 already in use owner@bugs.debian.org (Debian Bug Tracking System) - 2017-01-29 23:30 +0100
  Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Paul Menzel <paulepanter@users.sourceforge.net> - 2017-03-03 10:00 +0100
    Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Wolfram Sang <wsa@the-dreams.de> - 2017-03-03 11:30 +0100
      Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Paul Menzel <paulepanter@users.sourceforge.net> - 2017-03-31 09:30 +0200
        Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Guenter Roeck <linux@roeck-us.net> - 2017-03-31 15:20 +0200
          Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Boszormenyi Zoltan <zboszor@pr.hu> - 2017-03-31 16:50 +0200
            Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Guenter Roeck <linux@roeck-us.net> - 2017-03-31 17:10 +0200
              Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Boszormenyi Zoltan <zboszor@pr.hu> - 2017-04-01 12:20 +0200
                Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Guenter Roeck <linux@roeck-us.net> - 2017-04-01 15:40 +0200
                Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Boszormenyi Zoltan <zboszor@pr.hu> - 2017-04-01 18:30 +0200
                Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Boszormenyi Zoltan <zboszor@pr.hu> - 2017-04-01 18:40 +0200
                Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Paul Menzel <paulepanter@users.sourceforge.net> - 2017-04-03 08:40 +0200
                Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Boszormenyi Zoltan <zboszor@pr.hu> - 2017-04-03 10:10 +0200
                Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Boszormenyi Zoltan <zboszor@pr.hu> - 2017-06-27 14:30 +0200
  Bug#853122: Same Error Carl N <carlnikolov@gmail.com> - 2017-08-31 04:40 +0200
  Bug#853122: Same error sp5100_tco: I/O address 0x0cd6 already in use Sisim Biva <sisimbiva@gmail.com> - 2017-10-02 10:20 +0200
  Bug#853122: posts bugs alain BELLEC <alain_bellec@bbox.fr> - 2017-10-02 12:30 +0200
  Bug#853122: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Gyorgy Kapolnas <rty042@gmail.com> - 2017-11-22 12:30 +0100
  Bug#853122: linux-image-4.13.0-0.bpo.1-amd64: Also present at Gigabyte GA-AM1M-S2H Rainer Dorsch <ml@bokomoko.de> - 2017-12-16 21:30 +0100

csiph-web