Re: [PATCH v14 3/4] PCI: Bring the PCIe speed to MBps logic to new pcie_link_speed_to_mbps()

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

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

On Wed, 19 Jun 2024, Krishna Chaitanya Chundru wrote:

> 
> 
> On 6/14/2024 6:02 PM, Ilpo Järvinen wrote:
> > On Sun, 9 Jun 2024, Krishna chaitanya chundru wrote:
> > 
> > > Bring the switch case in pcie_link_speed_mbps() to new function to
> > > the header file so that it can be used in other places like
> > > in controller driver.
> > > 
> > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >   drivers/pci/pci.c | 19 +------------------
> > >   drivers/pci/pci.h | 22 ++++++++++++++++++++++
> > >   2 files changed, 23 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index d2c388761ba9..6e50fa89b913 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -6027,24 +6027,7 @@ int pcie_link_speed_mbps(struct pci_dev *pdev)
> > >   	if (err)
> > >   		return err;
> > >   -	switch (to_pcie_link_speed(lnksta)) {
> > > -	case PCIE_SPEED_2_5GT:
> > > -		return 2500;
> > > -	case PCIE_SPEED_5_0GT:
> > > -		return 5000;
> > > -	case PCIE_SPEED_8_0GT:
> > > -		return 8000;
> > > -	case PCIE_SPEED_16_0GT:
> > > -		return 16000;
> > > -	case PCIE_SPEED_32_0GT:
> > > -		return 32000;
> > > -	case PCIE_SPEED_64_0GT:
> > > -		return 64000;
> > > -	default:
> > > -		break;
> > > -	}
> > > -
> > > -	return -EINVAL;
> > > +	return pcie_link_speed_to_mbps(to_pcie_link_speed(lnksta));
> > 
> > pcie_link_speed_mbps() calls pcie_link_speed_to_mbps(), seems quite
> > confusing to me. Perhaps renaming one to pcie_dev_speed_mbps() would help
> > against the almost identical naming.
> > 
> > In general, I don't like moving that code into a header file, did you
> > check how large footprint the new function is (when it's not inlined)?
> > 
> if we remove this patch we see difference of 8, I think it should be fine.
> with change
> aarch64-linux-gnu-size ../drivers/pci/pci.o
>    text    data     bss     dec     hex filename
>   41440    1334      64   42838    a756 ../kobj/drivers/pci/pci.o
> without the change
> text    data     bss     dec     hex filename
>   41432    1334      64   42830    a74e ../kobj/drivers/pci/pci.o

Thanks for checking it out, seems the inline here was a non-problem.

-- 
 i.

> > Unrelated to this patch, it would be nice if LNKSTA register read would
> > not be needed at all here but since cur_bus_speed is what it is currently,
> > it's just wishful thinking.
> > 
> > >   }
> > >   EXPORT_SYMBOL(pcie_link_speed_mbps);
> > >   diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 1b021579f26a..391a5cd388bd 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -333,6 +333,28 @@ void pci_bus_put(struct pci_bus *bus);
> > >   	 (speed) == PCIE_SPEED_2_5GT  ?  2500*8/10 : \
> > >   	 0)
> > >   +static inline int pcie_link_speed_to_mbps(enum pci_bus_speed speed)
> > > +{
> > > +	switch (speed) {
> > > +	case PCIE_SPEED_2_5GT:
> > > +		return 2500;
> > > +	case PCIE_SPEED_5_0GT:
> > > +		return 5000;
> > > +	case PCIE_SPEED_8_0GT:
> > > +		return 8000;
> > > +	case PCIE_SPEED_16_0GT:
> > > +		return 16000;
> > > +	case PCIE_SPEED_32_0GT:
> > > +		return 32000;
> > > +	case PCIE_SPEED_64_0GT:
> > > +		return 64000;
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > 
> > 
> > 
>