linux-kernel - Re: [PATCH v14 3/4] PCI: Bring the PCIe speed to MBps logic to new pcie_link_speed_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Sun, 23 Jun 2024 18:26:03 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
cc: Bjorn Andersson <andersson@...nel.org>, 
    Konrad Dybcio <konrad.dybcio@...aro.org>, Rob Herring <robh@...nel.org>, 
    Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, 
    Conor Dooley <conor+dt@...nel.org>, 
    Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
    Lorenzo Pieralisi <lpieralisi@...nel.org>, 
    Krzysztof Wilczyński <kw@...ux.com>, 
    Bjorn Helgaas <bhelgaas@...gle.com>, johan+linaro@...nel.org, 
    bmasney@...hat.com, djakov@...nel.org, 
    Krzysztof Kozlowski <krzk+dt@...nel.org>, linux-arm-msm@...r.kernel.org, 
    devicetree@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, 
    linux-pci@...r.kernel.org, vireshk@...nel.org, quic_vbadigan@...cinc.com, 
    quic_skananth@...cinc.com, quic_nitegupt@...cinc.com, 
    quic_parass@...cinc.com, krzysztof.kozlowski@...aro.org
Subject: Re: [PATCH v14 3/4] PCI: Bring the PCIe speed to MBps logic to new
 pcie_link_speed_to_mbps()

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@...cinc.com>
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > > Acked-by: Bjorn Helgaas <bhelgaas@...gle.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;
> > > +}
> > 
> > 
> > 
>