Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Henning Schild <henning.schild@xxxxxxxxxxx>]

Am Wed, 13 Jul 2022 19:40:23 +0300
schrieb Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>:

> On Wed, Jun 29, 2022 at 07:14:06PM +0200, Henning Schild wrote:
> > Am Wed, 29 Jun 2022 19:39:58 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>:
> >   
> > > +Cc: Rafael
> > > 
> > > On Tue, Jun 21, 2022 at 02:58:16PM +0300, Andy Shevchenko wrote:  
> > > > On Wed, Jun 08, 2022 at 12:50:44PM +0200, Andy Shevchenko
> > > > wrote:    
> > > > > On Wed, Jun 8, 2022 at 9:42 AM Lee Jones
> > > > > <lee.jones@xxxxxxxxxx> wrote:    
> > > > > > On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> > > > > >    
> > > > > > > There are a few users that would like to utilize P2SB
> > > > > > > mechanism of hiding and unhiding a device from the PCI
> > > > > > > configuration space.
> > > > > > >
> > > > > > > Here is the series to consolidate p2sb handling code for
> > > > > > > existing users and to provide a generic way for new
> > > > > > > comer(s).
> > > > > > >
> > > > > > > It also includes a patch to enable GPIO controllers on
> > > > > > > Apollo Lake when it's used with ABL bootloader w/o ACPI
> > > > > > > support.
> > > > > > >
> > > > > > > The patch that brings the helper ("platform/x86/intel: Add
> > > > > > > Primary to Sideband (P2SB) bridge support") has a commit
> > > > > > > message that sheds a light on what the P2SB is and why
> > > > > > > this is needed.
> > > > > > >
> > > > > > > I have tested this on Apollo Lake platform (I'm able to
> > > > > > > see SPI NOR and since we have an ACPI device for GPIO I
> > > > > > > do not see any attempts to recreate one).
> > > > > > >
> > > > > > > The series is ready to be merged via MFD tree, but see
> > > > > > > below.
> > > > > > >
> > > > > > > The series also includes updates for Simatic IPC drivers
> > > > > > > that partially tagged by respective maintainers (the main
> > > > > > > question is if Pavel is okay with the last three patches,
> > > > > > > since I believe Hans is okay with removing some code
> > > > > > > under PDx86). Hence the first 8 patches can be merged
> > > > > > > right away and the rest when Pavel does his review.    
> > > > > >
> > > > > > Can we just wait for Pavel's review, then merge them all at
> > > > > > once?    
> > > > > 
> > > > > Sure, it would be the best course of action.    
> > > > 
> > > > Pavel, do you have a chance to review the patches (last three)
> > > > that touch LED drivers? What would be your verdict?    
> > > 
> > > Lee, Rafael,
> > > 
> > > It seems quite hard to get Pavel's attention to this series [1].
> > > It's already passed more than 3 weeks for any sign of review of
> > > three top patches of the series that touched LED subsystem. The
> > > entire series has all necessary tags, but for LED changes.
> > > 
> > > Note, that the top of this series is not done by me and was sent
> > > for preliminary review much earlier [2], altogether it makes
> > > months of no response from the maintainer.
> > > 
> > > The nature of patches is pretty simple and doesn't touch any of
> > > other than Simatic LED drivers nor LED core. Moreover, it was
> > > written by Siemens, who produces the H/W in question and very
> > > well tested as a separate change and as part of the series.  
> > 
> > The code has been reviewed and is in fact pretty simple. The only
> > questionable but pragmatic change that might catch the attention of
> > a pedantic reviewer is that i did put the gpio implementation of the
> > driver under the same/existing kernel config switch.
> >   
> > > I think to move forward we may ask Rafael to review it on behalf
> > > of good maintainer and with his approval apply entire series.
> > > 
> > > Thoughts?  
> > 
> > Thanks for pushing this Andy. I was wondering how and when that
> > story would continue. Technically these changes should really go in
> > one badge or we need to find a way to separate them somehow. I
> > would try to go that extra mile to get out of your way. But i am
> > kind of afraid such an effort might also end up touching the same
> > files and block us at the same maintainer.
> > 
> > Did anyone check whether Pavel was active at all in those last
> > months and maybe other patches waiting for review? Hope he is fine
> > and active and just somehow forgot/overlooked/ignored this one.  
> 
> I have send a private mail to Pavel and have got no response.
> Can we move this forward, let's say, by applying first 8 patches?

I am sorry that situation is now coming. Both simatic-ipc and that
appollo lake pinctrl driver compete for the same device memory. That
conflict was known and we agreed on sorting it out together somehow.
Not applying my patches could leave my LED drivers simply not working
any longer, or worse ... them making the apollolake platform stuff act
up somehow weird with unexpected EBUSY.

The series can not be split, or we have to write additional code to
properly deal with the conflict. I could envision my LED drivers still
accessing raw memory and ignoring EBUSY (very hacky! ... and touching
"we need Pavel code")

Another way could maybe be. Do the whole P2SB but do not make
apollolake pinctrl come up without ACPI. Somewhere in patches 1-8 there
is code which makes the pinctrl stuff come up for certain CPUs without
ACPI. It is really only some out of many CPUs which have pinctrl, and i
am not sure i remember what that has to do with the P2SB helpers as
such. The helpers are a refactoring, while the "bring up apollolake
pinctrl at all times" is a functional change ... now causing conflict.

And maybe there is a way/process to escalate to another maintainer.
Does anyone even know what is going on with Pavel? 

Henning

> > > [1]:
> > > https://lore.kernel.org/all/20220606164138.66535-1-andriy.shevchenko@xxxxxxxxxxxxxxx/
> > > [2]:
> > > https://lore.kernel.org/linux-leds/20220513083652.974-1-henning.schild@xxxxxxxxxxx/
> > >  
> >   
>