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] [thread-next>] [day] [month] [year] [list]

Message-ID: <54428aa3-2178-45d0-83d3-0b6254347bb5@163.com>
Date: Fri, 24 Jan 2025 21:12:49 +0800
From: Hans Zhang <18255117159@....com>
To: Frank Li <Frank.li@....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 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


>> +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