Hi Jean and Guenter, > This is a gentle reminder to review my previous response when possible. Thanks! Quite some modern AMD laptops seem to suffer from slow touchpads and this patch is part of the fix [1]. So, if you could comment on Terry's questions, this is highly appreciated! Thanks and all the best, Wolfram [1] https://lore.kernel.org/r/CAPoEpV0ZSidL6aMXvB6LN1uS-3CUHS4ggT8RwFgmkzzCiYJ-XQ@xxxxxxxxxxxxxx > > Regards, > Terry > > On 12/13/21 11:48 AM, Terry Bowman wrote: > > Hi Jean and Guenter, > > > > Jean, Thanks for your responses. I added comments below. > > > > I added Guenter to this email because his input is needed for adding the same > > changes to the sp5100_tco driver. The sp5100_tco and piix4_smbus driver > > must use the same synchronization logic for the shared register. > > > > On 11/5/21 11:05, Jean Delvare wrote: > >> On Tue, 7 Sep 2021 18:37:20 +0200, Jean Delvare wrote: > >>> More generally, I am worried about the overall design. The driver > >>> originally used per-access I/O port requesting because keeping the I/O > >>> ports busy all the time would prevent other drivers from working. Do we > >>> still need to do the same with the new code? If it is possible and safe > >>> to have a permanent mapping to the memory ports, that would be a lot > >>> faster. > >>> > > > > Permanent mapping would likely improve performance but will not provide the > > needed synchronization. As you mentioned below the sp5100 driver only uses > > the DECODEEN register during initialization but the access must be > > synchronized or an i2c transaction or sp5100_tco timer enable access may be > > lost. I considered alternatives but most lead to driver coupling or considerable > > complexity. > > > >>> On the other hand, the read-modify-write cycle in > >>> piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call > >>> request_mem_region() on the same memory area successfully. > >>> > >>> I'm not opposed to taking your patch with minimal changes (as long as > >>> the code is safe) and working on performance improvements later. > >> > > > > I confirmed through testing the request_mem_region() and request_muxed_region() > > macros provide exclusive locking. One difference between the 2 macros is the > > flag parameter, IORESOURCE_MUXED. request_muxed_region() uses the > > IORESOURCE_MUXED flag to retry the region lock if it's already locked. > > request_mem_region() does not use the IORESOURCE_MUXED and as a result will > > return -EBUSY immediately if the region is already locked. > > > > I must clarify: the piix4_smbus v1 patch uses request_mem_region() which is not > > correct because it doesn't retry locking an already locked region. The driver > > must support retrying the lock or piix4_smbus and sp5100_tco drivers may > > potentially fail loading. I added proposed piix4_smbus v2 changes below to solve. > > > > I propose reusing the existing request_*() framework from include/linux/ioport.h > > and kernel/resource.c. A new helper macro will be required to provide an > > interface to the "muxed" iomem locking functionality already present in > > kernel/resource.c. The new macro will be similar to request_muxed_region() > > but will instead operate on iomem. This should provide the same performance > > while using the existing framework. > > > > My plan is to add the following to include/linux/ioport.h in v2. This macro > > will add the interface for using "muxed" iomem support: > > #define request_mem_muxed_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED) > > > > The proposed changes will need review from more than one subsystem maintainer. > > The macro addition in include/linux/ioport.h would reside in a > > different maintainer's tree than this driver. The change to use the > > request_mem_muxed_region() macro will also be made to the sp5100_tco driver. > > The v2 review will include maintainers from subsystems owning piix4_smbus > > driver, sp5100_tco driver, and include/linux/ioport.h. > > > > The details provided above are described in a piix4_smbus context but would also be > > applied to the sp5100_tco driver for synchronizing the shared register. > > > > Jean and Guenter, do you have concerns or changes you prefer to the proposal I > > described above? > > > >> I looked some more at the code. I was thinking that maybe if the > >> registers accessed by the two drivers (i2c-piix4 and sp5100_tco) were > >> disjoint, then each driver could simply request subsets of the mapped > >> memory. > >> > >> Unfortunately, while most registers are indeed exclusively used by one > >> of the drivers, there's one register (0x00 = IsaDecode) which is used > >> by both. So this simple approach isn't possible. > >> > >> That being said, the register in question is only accessed at device > >> initialization time (on the sp5100_tco side, that's in function > >> sp5100_tco_setupdevice) and only for some devices (Embedded FCH). So > >> one approach which may work is to let the i2c-piix4 driver instantiate > >> the watchdog platform device in that case, instead of having sp5100_tco > >> instantiate its own device as is currently the case. That way, the > >> i2c-piix4 driver would request the "shared" memory area, perform the > >> initialization steps for both functions (SMBus and watchdog) and then > >> instantiate the watchdog device so that sp5100_tco gets loaded and goes > >> on with the runtime management of the watchdog device. > >> > >> If I'm not mistaken, this is what the i2c-i801 driver is already doing > >> for the watchdog device in all recent Intel chipsets. So maybe the same > >> approach can work for the i2c-piix4 driver for the AMD chipsets. > >> However I must confess that I did not try to do it nor am I familiar > >> with the sp5100_tco driver details, so maybe it's not possible for some > >> reason. > >> > >> If it's not possible then the only safe approach would be to migrate > >> i2c-piix4 and sp5100_tco to a true MFD setup with 3 separate drivers: > >> one new MFD PCI driver binding to the PCI device, providing access to > >> the registers with proper locking, and instantiating the platform > >> device, one driver for SMBus (basically i2c-piix4 converted to a > >> platform driver and relying on the MFD driver for register access) and > >> one driver for the watchdog (basically sp5100_tco converted to a > >> platform driver and relying on the MFD driver for register access). > >> That's a much larger change though, so I suppose we'd try avoid it if > >> at all possible. > >>
Attachment:
signature.asc
Description: PGP signature
