lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 1 Feb 2022 09:21:08 -0600 From: Terry Bowman <Terry.Bowman@....com> To: Andy Shevchenko <andy.shevchenko@...il.com> Cc: Guenter Roeck <linux@...ck-us.net>, linux-watchdog@...r.kernel.org, Jean Delvare <jdelvare@...e.com>, linux-i2c <linux-i2c@...r.kernel.org>, Wolfram Sang <wsa@...nel.org>, "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Wim Van Sebroeck <wim@...ux-watchdog.org>, Robert Richter <rrichter@....com>, Tom Lendacky <thomas.lendacky@....com>, sudheesh.mavila@....com, "Shah, Nehal-bakulchandra" <Nehal-bakulchandra.Shah@....com>, Basavaraj Natikar <Basavaraj.Natikar@....com>, Shyam Sundar S K <Shyam-sundar.S-k@....com>, Mario Limonciello <Mario.Limonciello@....com> Subject: Re: [PATCH v4 0/9] i2c: piix4: Replace cd6h/cd7h port I/O accesses with MMIO accesses Hi Andy, On 1/31/22 05:01, Andy Shevchenko wrote: > On Sun, Jan 30, 2022 at 8:41 PM Terry Bowman <terry.bowman@....com> wrote: >> >> This series changes the piix4_smbus driver's cd6h/cd7h port I/O accesses >> to use MMIO instead. This is necessary because cd6h/cd7h port I/O may be >> disabled on later AMD processors. >> >> This series includes patches with MMIO accesses to register >> FCH::PM::DECODEEN. The same register is also accessed by the sp5100_tco >> driver.[1] Synchronization to the MMIO register is required in both >> drivers. >> >> The first patch creates a macro to request MMIO region using the 'muxed' >> retry logic. This is used in patch 6 to synchronize accesses to EFCH MMIO. >> >> The second patch replaces a hardcoded region size with a #define. This is >> to improve maintainability and was requested from v2 review. >> >> The third patch moves duplicated region request/release code into >> functions. This locates related code into functions and reduces code line >> count. This will also make adding MMIO support in patch 6 easier. >> >> The fourth patch moves SMBus controller address detection into a function. >> This is in preparation for adding MMIO region support. >> >> The fifth patch moves EFCH port selection into a function. This is in >> preparation for adding MMIO region support. >> >> The sixth patch adds MMIO support for region requesting/releasing and >> mapping. This is necessary for using MMIO to detect SMBus controller >> address, enable SMBbus controller region, and control the port select. >> >> The seventh patch updates the SMBus controller address detection to support >> using MMIO. This is necessary because the driver accesses register >> FCH::PM::DECODEEN during initialization and only available using MMIO on >> later AMD processors. >> >> The eighth patch updates the SMBus port selection to support MMIO. This is >> required because port selection control resides in the >> FCH::PM::DECODEEN[smbus0sel] and is only accessible using MMIO on later AMD >> processors. >> >> The ninth patch enables the EFCH MMIO functionality added earlier in this >> series. The SMBus controller's PCI revision ID is used to check if EFCH >> MMIO is supported by HW and should be enabled in the driver. > > I'm not against the series, but I am still concerned that we are using > _p IO without clear understanding why. With cleaning them up, this can > be simpler and cleaner. > > For the i2c patches: > Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com> > if it helps somebody. > Thanks for the review Andy! I plan to add the ioport_map changes you recommend in a future patchset. I will include you as "suggested-by". Regards, Terry >> Based on v5.16. > > v5.17-rc2 is out :-) > >> >> Testing: >> Tested on family 19h using: >> i2cdetect -y 0 >> i2cdetect -y 2 >> >> - Results using v5.16 and this pachset applied. Below >> shows the devices detected on the busses: >> >> # i2cdetect -y 0 >> 0 1 2 3 4 5 6 7 8 9 a b c d e f >> 00: -- -- -- -- -- -- -- -- >> 10: 10 11 -- -- -- -- -- -- 18 -- -- -- -- -- -- -- >> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 30: 30 -- -- -- -- 35 36 -- -- -- -- -- -- -- -- -- >> 40: -- -- -- -- -- -- -- -- -- -- 4a -- -- -- -- -- >> 50: 50 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 70: -- -- -- 73 -- -- -- -- >> # i2cdetect -y 2 >> 0 1 2 3 4 5 6 7 8 9 a b c d e f >> 00: -- -- -- -- -- -- -- -- >> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 40: -- -- -- -- -- -- -- -- -- -- -- -- 4c -- -- -- >> 50: -- 51 -- -- 54 -- -- -- -- -- -- -- -- -- -- -- >> 60: 60 -- -- 63 -- -- 66 -- -- -- -- 6b -- -- 6e -- >> 70: 70 71 72 73 74 75 -- 77 >> >> Also tested using sp5100_tco submitted series listed below.[1] >> I applied the sp5100_tco v4 series and ran: >> cat >> /dev/watchdog >> >> [1] sp5100_tco v4 patchset is in process but not sent yet. Below is v3 >> upstream review: >> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-watchdog%2F20220118202234.410555-1-terry.bowman%40amd.com%2F&data=04%7C01%7Cterry.bowman%40amd.com%7Ca4a101d574724ba6958808d9e4a94579%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637792237972109034%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=LNxoYDcBRCT4Fih%2F8v1m9Xwbvbq1rqHbFlAcqrT4YDU%3D&reserved=0 >> >> Changes in v4: >> - Changed request_muxed_mem_region() macro to request_mem_region_muxed() >> in patch 1. (Andy Shevchenko) >> - Removed unnecessary newline where possible in calls to >> request_muxed_region() in patch 2. (Terry Bowman) >> - Changed piix4_sb800_region_setup() to piix4_sb800_region_request(). >> Patch 3. (Jean Delvare) >> - Reordered piix4_setup_sb800() local variables from longest name length >> to shortest name length. Patch 4. (Terry Bowman) >> - Changed piix4_sb800_region_request() and piix4_sb800_region_release() by >> adding early return() to remove 'else' improving readability. Patch 6. >> (Terry Bowman) >> - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is >> already enabled. (Terry Bowman) >> - Refactored piix4_sb800_port_sel() to simplify the 'if' statement using >> temp variable. Patch 8. (Terry Bowman) >> - Added mmio_cfg.use_mmio assignment in piix4_add_adapter(). This is >> needed for calls to piix4_sb800_port_sel() after initialization during >> normal operation. Patch 9. (Terry Bowman) >> >> Changes in v3: >> - Added request_muxed_mem_region() patch (Wolfram, Guenter) >> - Reduced To/Cc list length. (Andy) >> >> Changes in v2: >> - Split single patch. (Jean Delvare) >> - Replace constant 2 with SB800_PIIX4_SMB_MAP_SIZE where appropriate. >> (Jean Delvare) >> - Shorten SB800_PIIX4_FCH_PM_DECODEEN_MMIO_EN name length to >> SB800_PIIX4_FCH_PM_DECODEEN_MMIO. (Jean Delvare) >> - Change AMD_PCI_SMBUS_REVISION_MMIO from 0x59 to 0x51. (Terry Bowman) >> - Change piix4_sb800_region_setup() to piix4_sb800_region_request(). >> (Jean Delvare) >> - Change 'SMB' text in logging to 'SMBus' (Jean Delvare) >> - Remove unnecessary NULL assignment in piix4_sb800_region_release(). >> (Jean Delvare) >> - Move 'u8' variable definitions to single line. (Jean Delvare) >> - Hardcode piix4_setup_sb800_smba() return value to 0 since it is always >> 0. (Jean Delvare) >> >> Terry Bowman (9): >> kernel/resource: Introduce request_mem_region_muxed() >> i2c: piix4: Replace hardcoded memory map size with a #define >> i2c: piix4: Move port I/O region request/release code into functions >> i2c: piix4: Move SMBus controller base address detect into function >> i2c: piix4: Move SMBus port selection into function >> i2c: piix4: Add EFCH MMIO support to region request and release >> i2c: piix4: Add EFCH MMIO support to SMBus base address detect >> i2c: piix4: Add EFCH MMIO support for SMBus port select >> i2c: piix4: Enable EFCH MMIO for Family 17h+ >> >> drivers/i2c/busses/i2c-piix4.c | 207 ++++++++++++++++++++++++++------- >> include/linux/ioport.h | 2 + >> 2 files changed, 164 insertions(+), 45 deletions(-) >> >> -- >> 2.30.2 > > >
Powered by blists - more mailing lists