diff mbox

[net-next,02/14] sfc: Add sysfs entry for flags (link control and primary)

Message ID 556838ED.8020407@solarflare.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Shradha Shah May 29, 2015, 10:01 a.m. UTC 
On  every adapter there will be one primary PF per adaptor and
one link control PF per port.

Signed-off-by: Shradha Shah <sshah@solarflare.com>
---
 drivers/net/ethernet/sfc/ef10.c | 62 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 52 insertions(+), 10 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Laight May 29, 2015, 10:48 a.m. UTC | #1 
From: Shradha Shah
> Sent: 29 May 2015 11:01
> On  every adapter there will be one primary PF per adaptor and
> one link control PF per port.
...
> +	return sprintf(buf, "%d\n",
> +		       ((efx->mcdi->fn_flags) &
> +			(1 << MC_CMD_DRV_ATTACH_EXT_OUT_FLAG_LINKCTRL))
> +		       ? 1 : 0);

Horrid expression.
Why not:
	(efx->mcdi->fn_flags >> MC_CMD_DRV_ATTACH_EXT_OUT_FLAG_LINKCTRL) & 1

using sprintf() is also excessive. Maybe:
	*buf = '0' + (expression);
	return 1;

You may also need to check for buffer overrun.

	David

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Edward Cree May 29, 2015, 1:09 p.m. UTC | #2 
On 29/05/15 11:48, David Laight wrote:
> From: Shradha Shah
>> Sent: 29 May 2015 11:01
>> On  every adapter there will be one primary PF per adaptor and
>> one link control PF per port.
> ...
>> +	return sprintf(buf, "%d\n",
>> +		       ((efx->mcdi->fn_flags) &
>> +			(1 << MC_CMD_DRV_ATTACH_EXT_OUT_FLAG_LINKCTRL))
>> +		       ? 1 : 0);
> Horrid expression.
> Why not:
> 	(efx->mcdi->fn_flags >> MC_CMD_DRV_ATTACH_EXT_OUT_FLAG_LINKCTRL) & 1
I think the idea is that this is more explicit about what it's doing.
It's a toss-up which is more readable / idiomatic; I prefer the OP version.
(They probably compile to the same thing, though I haven't checked.)
> using sprintf() is also excessive. Maybe:
> 	*buf = '0' + (expression);
> 	return 1;
That loses the '\n'; it's annoying when you cat a file and it doesn't end in a '\n', because it gloms onto your shell prompt.
sprintf isn't really that expensive, this isn't likely to be called very frequently.
> You may also need to check for buffer overrun.
In fact Documentation/filesystems/sysfs.txt says that "show() should always use scnprintf()" and that "The buffer will always be PAGE_SIZE bytes in length."
So if we want to be consistent, it should be

	return scnprintf(buf, PAGE_SIZE, "%d\n", expression);

although it'd be rather surprising if either 0\n or 1\n were ever too big for PAGE_SIZE :grin:.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index ee20d96..ebdf6ee 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -255,8 +255,34 @@  static ssize_t efx_ef10_show_physical_port(struct device *dev,
 	return sprintf(buf, "%d\n", efx->port_num);
 }
 
-static DEVICE_ATTR(physical_port, 0444, efx_ef10_show_physical_port,
+static ssize_t efx_ef10_show_link_control_flag(struct device *dev,
+					       struct device_attribute *attr,
+					       char *buf)
+{
+	struct efx_nic *efx = pci_get_drvdata(to_pci_dev(dev));
+
+	return sprintf(buf, "%d\n",
+		       ((efx->mcdi->fn_flags) &
+			(1 << MC_CMD_DRV_ATTACH_EXT_OUT_FLAG_LINKCTRL))
+		       ? 1 : 0);
+}
+
+static ssize_t efx_ef10_show_primary_flag(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct efx_nic *efx = pci_get_drvdata(to_pci_dev(dev));
+
+	return sprintf(buf, "%d\n",
+		       ((efx->mcdi->fn_flags) &
+			(1 << MC_CMD_DRV_ATTACH_EXT_OUT_FLAG_PRIMARY))
+		       ? 1 : 0);
+}
+
+static DEVICE_ATTR(physical_port, 0444, efx_ef10_show_physical_port, NULL);
+static DEVICE_ATTR(link_control_flag, 0444, efx_ef10_show_link_control_flag,
 		   NULL);
+static DEVICE_ATTR(primary_flag, 0444, efx_ef10_show_primary_flag, NULL);
 
 static int efx_ef10_probe(struct efx_nic *efx)
 {
@@ -323,32 +349,41 @@  static int efx_ef10_probe(struct efx_nic *efx)
 	if (rc)
 		goto fail3;
 
-	rc = efx_ef10_get_pf_index(efx);
+	rc = device_create_file(&efx->pci_dev->dev,
+				&dev_attr_link_control_flag);
 	if (rc)
 		goto fail3;
 
+	rc = device_create_file(&efx->pci_dev->dev, &dev_attr_primary_flag);
+	if (rc)
+		goto fail4;
+
+	rc = efx_ef10_get_pf_index(efx);
+	if (rc)
+		goto fail5;
+
 	rc = efx_ef10_init_datapath_caps(efx);
 	if (rc < 0)
-		goto fail3;
+		goto fail5;
 
 	efx->rx_packet_len_offset =
 		ES_DZ_RX_PREFIX_PKTLEN_OFST - ES_DZ_RX_PREFIX_SIZE;
 
 	rc = efx_mcdi_port_get_number(efx);
 	if (rc < 0)
-		goto fail3;
+		goto fail5;
 	efx->port_num = rc;
 	rc = device_create_file(&efx->pci_dev->dev, &dev_attr_physical_port);
 	if (rc)
-		goto fail3;
+		goto fail5;
 
 	rc = efx->type->get_mac_address(efx, efx->net_dev->perm_addr);
 	if (rc)
-		goto fail4;
+		goto fail6;
 
 	rc = efx_ef10_get_sysclk_freq(efx);
 	if (rc < 0)
-		goto fail4;
+		goto fail6;
 
 	efx->timer_quantum_ns = 1536000 / rc; /* 1536 cycles */
 
@@ -368,7 +403,7 @@  static int efx_ef10_probe(struct efx_nic *efx)
 		nic_data->workaround_35388 = enabled &
 			MC_CMD_GET_WORKAROUNDS_OUT_BUG35388;
 	} else if (rc != -ENOSYS && rc != -ENOENT) {
-		goto fail4;
+		goto fail6;
 	}
 
 	netif_dbg(efx, probe, efx->net_dev,
@@ -377,14 +412,18 @@  static int efx_ef10_probe(struct efx_nic *efx)
 
 	rc = efx_mcdi_mon_probe(efx);
 	if (rc && rc != -EPERM)
-		goto fail4;
+		goto fail6;
 
 	efx_ptp_probe(efx, NULL);
 
 	return 0;
 
-fail4:
+fail6:
 	device_remove_file(&efx->pci_dev->dev, &dev_attr_physical_port);
+fail5:
+	device_remove_file(&efx->pci_dev->dev, &dev_attr_primary_flag);
+fail4:
+	device_remove_file(&efx->pci_dev->dev, &dev_attr_link_control_flag);
 fail3:
 	efx_mcdi_fini(efx);
 fail2:
@@ -629,6 +668,9 @@  static void efx_ef10_remove(struct efx_nic *efx)
 
 	device_remove_file(&efx->pci_dev->dev, &dev_attr_physical_port);
 
+	device_remove_file(&efx->pci_dev->dev, &dev_attr_primary_flag);
+	device_remove_file(&efx->pci_dev->dev, &dev_attr_link_control_flag);
+
 	efx_mcdi_fini(efx);
 	efx_nic_free_buffer(efx, &nic_data->mcdi_buf);
 	kfree(nic_data);