Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933255AbdCaPFb (ORCPT ); Fri, 31 Mar 2017 11:05:31 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:54442 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932934AbdCaPF3 (ORCPT ); Fri, 31 Mar 2017 11:05:29 -0400 Date: Fri, 31 Mar 2017 08:05:24 -0700 From: Guenter Roeck To: Boszormenyi Zoltan Cc: Paul Menzel , Wolfram Sang , Christian Fetzer , Jean Delvare , linux-i2c@vger.kernel.org, linux-watchdog@vger.kernel.org, 853122@bugs.debian.org, Wim Van Sebroeck , Tim Small , Nehal Shah , Mika Westerberg , Andy Shevchenko , Thomas Brandon , Eddi De Pieri , linux-kernel@vger.kernel.org Subject: Re: [Regression] Changes to i2c-piix4.c initialisation prevent loading of sp5100_tco watchdog driver on AMD SB800 chipset Message-ID: <20170331150524.GA31555@roeck-us.net> References: <1485728348.3220.10.camel@googlemail.com> <1488530782.2457.41.camel@users.sourceforge.net> <20170303101702.GA1669@katana> <1490944639.2653.182.camel@users.sourceforge.net> <585a27fb-be15-df9f-00a9-bd4c3a654142@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-Authenticated_sender: guenter@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: guenter@roeck-us.net X-Authenticated-Sender: bh-25.webhostbox.net: guenter@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5880 Lines: 145 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 >