Re: [PATCH v9 7/9] PCI/bwctrl: Add API to set PCIe Link Speed

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 18 Nov 2024 15:17:53 +0200 (EET)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Mon, 18 Nov 2024, Jonathan Cameron wrote:
> 
> > On Tue, 12 Nov 2024 18:01:50 +0200 (EET)
> > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> >   
> > > On Tue, 12 Nov 2024, Lukas Wunner wrote:
> > >   
> > > > On Fri, Oct 18, 2024 at 05:47:53PM +0300, Ilpo Järvinen wrote:    
> > > > > +EXPORT_SYMBOL_GPL(pcie_set_target_speed);    
> > > > 
> > > > My apologies for another belated comment on this series.
> > > > This patch is now a688ab21eb72 on pci/bwctrl:
> > > > 
> > > > I note that pcie_set_target_speed() is not called my a modular user
> > > > (CONFIG_PCIE_THERMAL is bool, not tristate), so the above-quoted export
> > > > isn't really necessary right now.  I don't know if it was added
> > > > intentionally because some modular user is expected to show up
> > > > in the near future.    
> > > 
> > > Its probably a thinko to add it at all but then there have been talk about 
> > > other users interested in the API too so it's not far fetched we could see 
> > > a user. No idea about timelines though.
> > > 
> > > There are some AMD GPU drivers tweaking the TLS field on their own but 
> > > they also touch some HW specific registers (although, IIRC, they only 
> > > touch Endpoint'sTLS). I was thinking of converting them but I'm unsure if 
> > > that yields something very straightforward and ends up producing a working 
> > > conversion or not (without ability to test with the HW). But TBH, not on 
> > > my highest priority item.
> > >   
> > > > > @@ -135,6 +296,7 @@ static int pcie_bwnotif_probe(struct pcie_device *srv)
> > > > >  	if (!data)
> > > > >  		return -ENOMEM;
> > > > >  
> > > > > +	devm_mutex_init(&srv->device, &data->set_speed_mutex);
> > > > >  	ret = devm_request_threaded_irq(&srv->device, srv->irq, NULL,
> > > > >  					pcie_bwnotif_irq_thread,
> > > > >  					IRQF_SHARED | IRQF_ONESHOT,    
> > > > 
> > > > We generally try to avoid devm_*() functions in port service drivers
> > > > because if we later on move them into the PCI core (which is the plan),
> > > > we'll have to unroll them.  Not the end of the world that they're used
> > > > here, just not ideal.    
> > > 
> > > I think Jonathan disagrees with you on that:
> > > 
> > > https://lore.kernel.org/linux-pci/20241017114812.00005e67@Huawei.com/  
> > 
> > Indeed - you beat me to it ;)
> > 
> > There is no practical way to move most of the port driver code into the PCI
> > core and definitely not interrupts. It is a shame though as I'd much prefer
> > if we could do so.  At LPC other issues some as power management were called
> > out as being very hard to handle, but to me the interrupt bit is a single
> > relatively easy to understand blocker.
> > 
> > I've been very slow on getting back to this, but my current plan would
> > 
> > 1) Split up the bits of portdrv subdrivers that are actually core code
> >    (things like the AER counters etc) The current mix in the same files
> >    makes it hard to reason about lifetimes etc.
> > 
> > 2) Squash all the portdrv sub drivers into simple library style calls so
> >    no pretend devices, everything registered directly.  That cleans up
> >    a lot of the layering and still provides reusable code if it makes
> >    sense to have multiple drivers for ports or to reuse this code for
> >    something else. Note that along the way I'll check we can build the
> >    portdrv as a module - I don't plan to actually do that, but it shows
> >    the layering / abstractions all work if it is possible.  That will
> >    probably make use of devm_ where appropriate as it simplifies a lot
> >    of paths.  
> 
> I'm sorry to be a bit of a spoilsport here but quirks make calls to ASPM 
> code and now also to bwctrl set Link Speed API so I'm not entire sure if 
> you can actual succeed in that module test.
> 
> (It's just something that again indicates both would belong to PCI core
> but sadly that direction is out of options).
> 
It may involve some bodges, but it is still a path to checking the
layer splits at least make 'some' sense.  Also that the resulting
library style code is suitable for reuse.  Possibly with an exception
for a few parts.

Thanks for the pointers to where the pitfalls lie!

Jonathan