[net-next-2.6 PATCH][be2net] Code changes to handle dev pvt ioctl

[net-next-2.6 PATCH][be2net] Code changes to handle dev pvt ioctl

[Sarveshwar Bandi]

Resubmitting patch for net-next tree to handle SIOCDEVPRIVATE ioctl. Also
have incorporated comments given by Ben Hutchings.

- Sarvesh

Signed-off-by: Sarveshwar Bandi <sarveshwarb@serverengines.com>
---
 drivers/net/benet/be_cmds.c |   20 ++++++++++++++++++
 drivers/net/benet/be_cmds.h |    2 ++
 drivers/net/benet/be_main.c |   49 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/drivers/net/benet/be_cmds.c b/drivers/net/benet/be_cmds.c
index d444aed..21263e2 100644
--- a/drivers/net/benet/be_cmds.c
+++ b/drivers/net/benet/be_cmds.c
@@ -859,3 +859,23 @@ int be_cmd_query_fw_cfg(struct be_ctrl_i
 	spin_unlock(&ctrl->cmd_lock);
 	return status;
 }
+
+int be_cmd_pass_ext_ioctl(struct be_ctrl_info *ctrl, dma_addr_t dma,
+			  int req_size)
+{
+	struct be_mcc_wrb *wrb = wrb_from_mbox(&ctrl->mbox_mem);
+	struct be_sge *sge = nonembedded_sgl(wrb);
+	int status;
+
+	spin_lock(&ctrl->cmd_lock);
+	memset(wrb, 0, sizeof(*wrb));
+	be_wrb_hdr_prepare(wrb, req_size, false, 1);
+	sge->pa_hi = cpu_to_le32(upper_32_bits(dma));
+	sge->pa_lo = cpu_to_le32(dma & 0xFFFFFFFF);
+	sge->len = cpu_to_le32(req_size);
+
+	status = be_mbox_db_ring(ctrl);
+
+	spin_unlock(&ctrl->cmd_lock);
+	return status;
+}
diff --git a/drivers/net/benet/be_cmds.h b/drivers/net/benet/be_cmds.h
index e499e2d..b147286 100644
--- a/drivers/net/benet/be_cmds.h
+++ b/drivers/net/benet/be_cmds.h
@@ -686,3 +686,5 @@ extern int be_cmd_set_flow_control(struc
 extern int be_cmd_get_flow_control(struct be_ctrl_info *ctrl,
 			u32 *tx_fc, u32 *rx_fc);
 extern int be_cmd_query_fw_cfg(struct be_ctrl_info *ctrl, u32 *port_num);
+extern int be_cmd_pass_ext_ioctl(struct be_ctrl_info *ctrl, dma_addr_t dma,
+			  int req_size);
diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index ae2f6b5..103bfa2 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -562,6 +562,54 @@ static void be_vlan_rem_vid(struct net_d
 	be_vid_config(netdev);
 }
 
+static int be_do_ioctl(struct net_device *netdev,
+			struct ifreq *ifr, int cmd)
+{
+	struct be_adapter *adapter = netdev_priv(netdev);
+	struct be_cmd_req_hdr req;
+	struct be_cmd_resp_hdr *resp;
+	void *data = ifr->ifr_data;
+	void *va = NULL;
+	dma_addr_t dma;
+	u32 req_size, resp_size;
+	int status, ret = 0;
+
+	switch (cmd) {
+	case SIOCDEVPRIVATE:
+		if (!capable(CAP_NET_ADMIN))
+			return -EPERM;
+
+		if (copy_from_user(&req, (struct be_cmd_req_hdr *)data,
+			sizeof(struct be_cmd_req_hdr)))
+			return -EFAULT;
+
+		req_size = req.request_length + sizeof(struct be_cmd_req_hdr);
+
+		va = pci_alloc_consistent(adapter->pdev, req_size, &dma);
+		if (!va)
+			return -ENOMEM;
+		if (copy_from_user(va, (void *)data, req_size)) {
+			ret = -EFAULT;
+			break;
+		}
+
+		status = be_cmd_pass_ext_ioctl(&adapter->ctrl, dma, req_size);
+		if (!status) {
+			resp = (struct be_cmd_resp_hdr *) va;
+			resp_size = resp->response_length + sizeof(*resp);
+			if (copy_to_user((void *)data, va, resp_size))
+				ret = -EFAULT;
+		} else
+			ret = -EFAULT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	if (va)
+		pci_free_consistent(adapter->pdev, req_size, va, dma);
+	return ret;
+}
+
 static void be_set_multicast_filter(struct net_device *netdev)
 {
 	struct be_adapter *adapter = netdev_priv(netdev);
@@ -1632,6 +1680,7 @@ static struct net_device_ops be_netdev_o
 	.ndo_vlan_rx_register	= be_vlan_register,
 	.ndo_vlan_rx_add_vid	= be_vlan_add_vid,
 	.ndo_vlan_rx_kill_vid	= be_vlan_rem_vid,
+	.ndo_do_ioctl		= be_do_ioctl,
 };
 
 static void be_netdev_init(struct net_device *netdev)
-- 
1.4.0

Re: [net-next-2.6 PATCH][be2net] Code changes to handle dev pvt ioctl

[David Miller]

From: Sarveshwar Bandi <sarveshwarb@serverengines.com>
Date: Sun, 24 May 2009 11:01:58 +0530

> Resubmitting patch for net-next tree to handle SIOCDEVPRIVATE ioctl. Also
> have incorporated comments given by Ben Hutchings.
> 
> Signed-off-by: Sarveshwar Bandi <sarveshwarb@serverengines.com>

So what kind of diagnostic commands do userlevel tools need
this for?

If they fall into useful generic categories, I'd rather you add new
ethtool commands for doing them, one for each diagnostic command.
For all I know, we might even have such things already if this is
for self-test, firmware upload/download, or similar.

Device private ioctls are heavily undesirable, because every driver
implements different ones, and thus the experience for users and tool
writers is completely inconsistent.

Until I know more about what this is going to be used for I am
rejecting this patch.

I really don't like any of the changes I've seen submitted recently
for the be2net driver, to be honest with you. :-/

Re: [net-next-2.6 PATCH][be2net] Code changes to handle dev pvt ioctl

[Sarveshwar Bandi]

The private ioctl is being implemented primarily to meet the following OEM
requirements.

1.  A mechanism to retrieve the last post mortem dump of the firmware and h/w
state.
2.  Fine tune port equalization parameters of the physical ports.
3.  A mechanism to flash the firmware at any point (not just at driver load
time).

A private ioctl seemed to be the only available mechanism to accomplish all
these.  Also we noticed that couple of other NIC drivers had implemented
private ioctls. If you feel there are better ways to do this, please let me
know.

- Sarvesh

On 25/05/09 00:34 -0700, David Miller wrote:
> From: Sarveshwar Bandi <sarveshwarb@serverengines.com>
> Date: Sun, 24 May 2009 11:01:58 +0530
> 
> > Resubmitting patch for net-next tree to handle SIOCDEVPRIVATE ioctl. Also
> > have incorporated comments given by Ben Hutchings.
> > 
> > Signed-off-by: Sarveshwar Bandi <sarveshwarb@serverengines.com>
> 
> So what kind of diagnostic commands do userlevel tools need
> this for?
> 
> If they fall into useful generic categories, I'd rather you add new
> ethtool commands for doing them, one for each diagnostic command.
> For all I know, we might even have such things already if this is
> for self-test, firmware upload/download, or similar.
> 
> Device private ioctls are heavily undesirable, because every driver
> implements different ones, and thus the experience for users and tool
> writers is completely inconsistent.
> 
> Until I know more about what this is going to be used for I am
> rejecting this patch.
> 
> I really don't like any of the changes I've seen submitted recently
> for the be2net driver, to be honest with you. :-/
> 
> --
> 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

Re: [net-next-2.6 PATCH][be2net] Code changes to handle dev pvt ioctl

[David Miller]

From: Sarveshwar Bandi <sarveshwarb@serverengines.com>
Date: Tue, 26 May 2009 16:18:53 +0530

> 
> The private ioctl is being implemented primarily to meet the following OEM
> requirements.
> 
> 1.  A mechanism to retrieve the last post mortem dump of the firmware and h/w
> state.

Belongs in ethtool.

> 2.  Fine tune port equalization parameters of the physical ports.

Belongs in the driver, definitely not in userspace via random pokes
via this new ioctl.  Maybe it would be OK to do this with a new very
well defined ethtool command.

> 3.  A mechanism to flash the firmware at any point (not just at driver load
> time).

Also belongs in ethtool.

All of this is about the typing of the interface.  What you're adding
with the private ioctl is a big black box that no application
developer can code to and have it work on other cards supporting
similar facilities.

We need consistent interfaces that any driver trying to do this can use,
not a different hack in every driver.