Re: [PATCH v3 09/10] thermal: Add PCIe cooling driver

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

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

On Sat, 30 Dec 2023, Lukas Wunner wrote:

> On Fri, Sep 29, 2023 at 02:57:22PM +0300, Ilpo Järvinen wrote:
> > @@ -253,8 +255,16 @@ static int pcie_bandwidth_notification_probe(struct pcie_device *srv)
> >  	pcie_enable_link_bandwidth_notification(port);
> >  	pci_info(port, "enabled with IRQ %d\n", srv->irq);
> >  
> > +	data->cdev = pcie_cooling_device_register(port, srv);
> > +	if (IS_ERR(data->cdev)) {
> > +		ret = PTR_ERR(data->cdev);
> > +		goto disable_notifications;
> > +	}
> >  	return 0;
> 
> Now wait a minute, if you can't register the cooling device,
> you still want to provide accurate link speeds to the user
> in sysfs, right?  At least that's what you promise in Kconfig.

When thermal side is not even configured, it returns NULL which is not 
ERR.

I guess I can change the behavior for the real ERR cases (I was bit on 
borderline what to do with those failures).

> > --- /dev/null
> > +++ b/drivers/thermal/pcie_cooling.c
> > @@ -0,0 +1,107 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * PCIe cooling device
> > + *
> > + * Copyright (C) 2023 Intel Corporation.
> 
> Another trailing period I'd remove.
> 
> I take it this patch (only) allows manual bandwidth throttling
> through sysfs, right?  And emergency throttling is introduced
> separately on top of this?

Only sysfs throttling is introduced by this series, there's no emergency 
throttling in the series. Also, many things have been simplified in this 
series because more complex things would be only justified with 
the emergency throttling and would just raise questions 'why is this and 
that being done' (e.g., the critical section was enlarged as per your 
request where if there would be emergency throttlink doesn't make sense to 
wait until the end of the link training before "emergency throttling" can 
attempt to lower the link speed).


-- 
 i.