Re: [PATCH 1/2] i2c: i801: Switch to iomapped register access

[Heiner Kallweit <hkallweit1@gmail.com>]

On 19.03.2025 22:53, Andi Shyti wrote:
> On Wed, Mar 19, 2025 at 09:26:35PM +0100, Heiner Kallweit wrote:
>> On 19.03.2025 20:48, Andy Shevchenko wrote:
>>> On Wed, Mar 19, 2025 at 9:33 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>> On 19.03.2025 09:23, Andy Shevchenko wrote:
>>>>> On Wed, Mar 19, 2025 at 9:17 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>>>> On 19.03.2025 00:22, Andi Shyti wrote:
>>>>>>> On Wed, Mar 19, 2025 at 12:18:47AM +0200, Andy Shevchenko wrote:
>>>>>>>> Wed, Mar 12, 2025 at 08:07:23PM +0100, Heiner Kallweit kirjoitti:
>>>>>
>>>>>>>>> Switch to iomapped register access as a prerequisite for adding
>>>>>>>>> support for MMIO register access.
>>>>>>>>
>>>>>>>> I believe that I at least discussed the similar change a few years ago or even
>>>>>>>> proposed a one. The problem here is that *_p() variants of IO port accessors
>>>>>>>> are not the same as non-_p ones. And commit message is kept silent about
>>>>>>>> possible consequences of this change.
>>>>>>>>
>>>>>>>> So, at bare minumum it would be good to test for some period of time before
>>>>>>>> going for it.
>>>>>>>
>>>>>>> How would you do it?
>>>>>>
>>>>>> Documentation/driver-api/device-io.rst states that the artificially delayed
>>>>>> _p versions were needed on ISA devices. And in general I didn't find any hint
>>>>>> that the non-delayed versions ever caused issues on PCI devices.
>>>>>
>>>>> At least put this in the commit message. It will show that you were aware of _p.
>>>>>
>>>>>> On my system using the non-delayed version works fine, but I can't say 100%
>>>>>> that it's the same for the very first (> 25 yrs ago) chipsets supported by i801.
>>>>>>
>>>>>> Likely users with old systems don't run -next kernels, therefore leaving
>>>>>> this change a full cycle in -next may not really help. We can argue that
>>>>>> we have the -rc period for testing (and reverting if needed).
>>>>>
>>>>> My main concern is to make no regressions for most currently used
>>>>> cases, that's why one cycle in Linux Next is better than none.
>>>>
>>>> Even ICH7 datasheet from 2012 mentions that SMBus register space is also
>>>> memory-mapped. So all systems from at least the last 10 yrs should use MMIO
>>>> instead of PMIO now, and therefore not be affected by switching to non-delayed
>>>> PMIO access. This should significantly reduce the risk you're referring to.
>>>
>>> Cool! So, can we just put a summary into the commit message of all
>>> findings, worries (or their absence)?
>>>
>> Sure. Would be a question to Andi how this should be done technically.
> 
> yes, please do and I will update the commit. You can even provide
> the new commit message in reply to this email and I will update
> the rest.
> 
Updated commit message for d4ac3f93ff23:

Switch to iomapped register access as a prerequisite for adding
support for MMIO register access.

This changes replaces the delayed inb_p/outb_p calls with calls to
ioread8/iowrite8 which don't have this extra delay. According to
Documentation/driver-api/device-io.rst the _p versions are needed
for ISA device access only, therefore switching to the non-delayed
versions should not cause problems. However a certain risk remains,
which on the other hand is significantly reduced by the fact that
recent systems will use MMIO instead of PIO. ICH7 datasheet from 2012
mentions already that SMBus register space is also memory-mapped.
So all systems from at least the last 10 yrs should be safe.


> Thanks,
> Andi

Heiner