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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]

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/

:-)

-- 
 i.