linux-kernel - Re: [PATCH] PCI: dwc: Add the sysfs property to provide the LTSSM status of the PCIe link

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]

Message-ID: <Z5O5VsMLYE9R+loz@lizhi-Precision-Tower-5810>
Date: Fri, 24 Jan 2025 11:01:26 -0500
From: Frank Li <Frank.li@....com>
To: Hans Zhang <18255117159@....com>
Cc: jingoohan1@...il.com, manivannan.sadhasivam@...aro.org,
	lpieralisi@...nel.org, kw@...ux.com, robh@...nel.org,
	bhelgaas@...gle.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, rockswang7@...il.com
Subject: Re: [PATCH] PCI: dwc: Add the sysfs property to provide the LTSSM
 status of the PCIe link

On Fri, Jan 24, 2025 at 09:12:49PM +0800, Hans Zhang wrote:
>
>
> On 2025/1/24 00:16, Frank Li wrote:
> > > +static char *dw_ltssm_sts_string(enum dw_pcie_ltssm ltssm)
> > > +{
> > > +	char *str;
> > > +
> > > +	switch (ltssm) {
> > > +#define DW_PCIE_LTSSM_NAME(n) case n: str = #n; break
> > > +	DW_PCIE_LTSSM_NAME(DW_PCIE_LTSSM_DETECT_QUIET);
> > > +	DW_PCIE_LTSSM_NAME(DW_PCIE_LTSSM_DETECT_ACT);
> > > +	DW_PCIE_LTSSM_NAME(DW_PCIE_LTSSM_POLL_ACTIVE);
> > > +	DW_PCIE_LTSSM_NAME(DW_PCIE_LTSSM_POLL_COMPLIANCE);
> > > +	...
> > > +	default:
> > > +		str = "DW_PCIE_LTSSM_UNKNOWN";
> > > +		break;
> >
> > I prefer
> > const char * str[] =
> > {
> > 	"detect_quitet",
> > 	"detect_act",
> > 	...
> > }
> >
> > 	return str[ltssm];
> >
> > Or
> > 	#define DW_PCIE_LTSSM_NAME(n) case n: return #n;
> > 	...
> > 	default:
> > 		return "DW_PCIE_LTSSM_UNKNOWN";
> Hi Frank,
>
> I considered the two methods you mentioned before I submitted this patch.
>
> The first, I think, will increase the memory overhead.
>
> +static const char * const dw_pcie_ltssm_str[] = {
> +	[DW_PCIE_LTSSM_DETECT_QUIET] = "DETECT_QUIET",
> +	[DW_PCIE_LTSSM_DETECT_ACT] = "DETECT_ACT",
> +	[DW_PCIE_LTSSM_POLL_ACTIVE] = "POLL_ACTIVE",
> +	[DW_PCIE_LTSSM_POLL_COMPLIANCE] = "POLL_COMPLIANCE",
> 	...
>
>
> The second, ./scripts/checkpatch.pl checks will have a warning
>
> WARNING: Macros with flow control statements should be avoided
> #121: FILE: drivers/pci/controller/dwc/pcie-designware.h:329:
> +#define DW_PCIE_LTSSM_NAME(n) case n: return #n
>

Okay, it is not big deal
can you return
	str + strlen("DW_PCIE_LTSSM_");

Or trim useless "DW_PCIE_LTSSM_" information.

Frank

>
> > > +static ssize_t ltssm_status_show(struct device *dev,
> > > +				 struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > +	struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus);
> > > +	struct dw_pcie_rp *pp = bridge->sysdata;
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +
> > > +	return sysfs_emit(buf, "%s\n",
> > > +			  dw_ltssm_sts_string(dw_pcie_get_ltssm(pci)));
> >
> > Suggest dump raw value also
> >
> > val = dw_pcie_get_ltssm(pci);
> > return sysfs_emit(buf, "%s (0x%02x)\n",
> > 		  dw_ltssm_sts_string(val), val);
>
> Thanks, i think it's a good idea.
>
> Best regards
> Hans
>