[PATCH 00/13] staging: unisys: visorbus and visornic fixes

[PATCH 00/13] staging: unisys: visorbus and visornic fixes

[Benjamin Romer]

This patch set contains fixes for visorbus and visornic, and adds
appropriate error messages to the visornic driver.

David Kershner (1):
  staging: unisys: Allow visorbus to autoload

Tim Sell (12):
  staging: unisys: respond to msgs post device_create
  staging: unisys: prevent faults processing messages
  staging: unisys: visornic: correct visornic_pause
  staging: unisys: prevent faults in visornic_pause
  staging: unisys: neglect to NULL rcvbuf pointer
  staging: unisys: add error messages to visornic
  staging: unisys: visornic: correct obvious double-allocation of
    workqueues
  staging: unisys: visornic: correctly clean up device on removal
  staging: unisys: visornic: don't destroy global workqueues until devs
    destroyed
  staging: unisys: visornic: delay start of worker thread until netdev
    created
  staging: unisys: visornic: use preferred interface for setting
    netdev's parent
  staging: unisys: visornic: prevent erroneous kfree of devdata pointer

 drivers/staging/unisys/include/visorbus.h       |   3 +-
 drivers/staging/unisys/visorbus/visorchipset.c  |   4 +
 drivers/staging/unisys/visornic/visornic_main.c | 225 ++++++++++++++++++------
 3 files changed, 176 insertions(+), 56 deletions(-)

-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 01/13] staging: unisys: respond to msgs post device_create

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Fix problem that prevents us from responding to any device message after
device_create.

By neglecting to NULL out pending_msg_hdr after the device_create response,
we were effectively preventing any subsequent messages to the device from
working, because device_epilog() will correctly bail out early if it sees
that pending_msg_hdr is still set non-NULL, as that is an indicator to mean
that an unanswered message is still outstanding.

This problem was discovered as part of testing IOVM service partition
recovery, because device_epilog() was in fact bailing out when it was
called from my_device_changestate(), which of course prevented us from
transitioning the device to the paused state.  However, the incorrect
behavior would occur for ANY subsequent command directed at the device,
not just for changestate.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorbus/visorchipset.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index bb8087e..d47ec24 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -2047,6 +2047,7 @@ device_create_response(struct visor_device *dev_info, int response)
 			 response);
 
 	kfree(dev_info->pending_msg_hdr);
+	dev_info->pending_msg_hdr = NULL;
 }
 
 static void
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 02/13] staging: unisys: prevent faults processing messages

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Prevent faults processing messages for devices that no driver has yet
registered to handle.

Previously, code of the form:

    drv = to_visor_driver(dev->device.driver);
    if (!drv)
        goto away;

was not having the desired intent, because to_visor_driver() was
essentially returning garbage if its argument was NULL.  The only existing
case of this is in initiate_chipset_device_pause_resume(), which is called
during IOVM service partition recovery.  We were thus faulting when IOVM
service partition recovery was initiated on a bus that had at least one
device for which no function driver had registered
(visorbus_register_visor_driver).

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/include/visorbus.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/include/visorbus.h b/drivers/staging/unisys/include/visorbus.h
index e4a21e4..a0144c6 100644
--- a/drivers/staging/unisys/include/visorbus.h
+++ b/drivers/staging/unisys/include/visorbus.h
@@ -113,7 +113,8 @@ struct visor_driver {
 	struct driver_attribute version_attr;
 };
 
-#define to_visor_driver(x) container_of(x, struct visor_driver, driver)
+#define to_visor_driver(x) ((x) ? \
+	(container_of(x, struct visor_driver, driver)) : (NULL))
 
 /** A device type for things "plugged" into the visorbus bus */
 
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 03/13] staging: unisys: visornic: correct visornic_pause

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Correct visornic_pause() to indicate completion asynchronously rather
than in-line

Previously, visornic_pause() (called to stop the device due to IOVM service
partition recovery) was calling the passed complete_func() in-line, rather
than delaying the calling until after the device had actually been stopped.

The behavior has been corrected so that the calling of the complete_func()
is now delayed until after the stopping of the device has been completed in
visornic_serverdown_complete(), which runs asynchronously via the workqueue
visornic_serverdown_workqueue.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 7100744..fa52e0f 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -151,6 +151,7 @@ struct visornic_devdata {
 					  * sent to the IOPART end
 					  */
 	struct work_struct serverdown_completion;
+	visorbus_state_complete_func server_down_complete_func;
 	struct work_struct timeout_reset;
 	struct uiscmdrsp *cmdrsp_rcv;	 /* cmdrsp_rcv is used for
 					  * posting/unposting rcv buffers
@@ -545,8 +546,12 @@ visornic_serverdown_complete(struct work_struct *work)
 	atomic_set(&devdata->num_rcvbuf_in_iovm, 0);
 	spin_unlock_irqrestore(&devdata->priv_lock, flags);
 
+	if (devdata->server_down_complete_func)
+		(*devdata->server_down_complete_func)(devdata->dev, 0);
+
 	devdata->server_down = true;
 	devdata->server_change_state = false;
+	devdata->server_down_complete_func = NULL;
 }
 
 /**
@@ -558,10 +563,12 @@ visornic_serverdown_complete(struct work_struct *work)
  *	Returns 0 if we scheduled the work, -EINVAL on error.
  */
 static int
-visornic_serverdown(struct visornic_devdata *devdata)
+visornic_serverdown(struct visornic_devdata *devdata,
+		    visorbus_state_complete_func complete_func)
 {
 	if (!devdata->server_down && !devdata->server_change_state) {
 		devdata->server_change_state = true;
+		devdata->server_down_complete_func = complete_func;
 		queue_work(visornic_serverdown_workqueue,
 			   &devdata->serverdown_completion);
 	} else if (devdata->server_change_state) {
@@ -895,7 +902,7 @@ visornic_timeout_reset(struct work_struct *work)
 	return;
 
 call_serverdown:
-	visornic_serverdown(devdata);
+	visornic_serverdown(devdata, NULL);
 }
 
 /**
@@ -1980,8 +1987,7 @@ static int visornic_pause(struct visor_device *dev,
 {
 	struct visornic_devdata *devdata = dev_get_drvdata(&dev->device);
 
-	visornic_serverdown(devdata);
-	complete_func(dev, 0);
+	visornic_serverdown(devdata, complete_func);
 	return 0;
 }
 
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 04/13] staging: unisys: prevent faults in visornic_pause

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Prevent faults in visornic_pause, visornic_resume(), and visornic_remove()

Prior to this patch, any call to visornic_pause(), visornic_resume(), or
visornic_remove() would fault, due to dev_set_drvdata() never having been
called to stash our struct visornic_devdata * into the device's drvdata.
I.e., all calls to dev_get_drvdata() were returning NULL, meaning a fault
was soon to follow.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index fa52e0f..72253a0 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1831,6 +1831,7 @@ static int visornic_probe(struct visor_device *dev)
 	}
 
 	devdata->netdev = netdev;
+	dev_set_drvdata(&dev->device, devdata);
 	init_waitqueue_head(&devdata->rsp_queue);
 	spin_lock_init(&devdata->priv_lock);
 	devdata->enabled = 0; /* not yet */
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 05/13] staging: unisys: neglect to NULL rcvbuf pointer

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Neglect to NULL rcvbuf pointer array could result in faults later

This problem would exhibit itself as a fault when when attempting to stop
any visornic device (i.e., in visornic_disable_with_timeout() or
visornic_serverdown_complete()) that had never been started (i.e., for
which init_rcv_bufs() had never been called).  Because the array of rcvbuf
was never cleared to NULLs, we would mistakenly attempt to call kfree_skb()
on garbage memory.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 72253a0..915c913 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1845,7 +1845,7 @@ static int visornic_probe(struct visor_device *dev)
 	if (err)
 		goto cleanup_netdev;
 
-	devdata->rcvbuf = kmalloc(sizeof(struct sk_buff *) *
+	devdata->rcvbuf = kzalloc(sizeof(struct sk_buff *) *
 				  devdata->num_rcv_bufs, GFP_KERNEL);
 	if (!devdata->rcvbuf) {
 		err = -ENOMEM;
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 06/13] staging: unisys: add error messages to visornic

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Add error message to genuine rare error paths, and debug messages
to enable relatively non-verbose tracing of code paths

You can enable debug messages by including this on the kernel command line:

    visornic.dyndbg=+p

or by this from the command line:

    echo "module visornic +p" > <debugfs>/dynamic_debug/control

Refer to Documentation/dynamic-debug-howto.txt for more details.

In addition to the new debug and error messages, a message like the
following will be logged every time a visornic device is probed, which
will enable you to map back-and-forth between visorbus device names
(e.g., "vbus2:dev0") and netdev names (e.g., "eth0"):

    visornic vbus2:dev0: visornic_probe success netdev=eth0

With this patch and visornic debugging enabled, you should expect to see
messages like the following in the most-common scenarios:

* driver loaded:

      visornic_init

* device probed:

      visornic vbus2:dev0: visornic_probe
      visor_thread_start
      visor_thread_start success

* network interface configured (ifconfig):

      net eth0: visornic_open
      net eth0: visornic_enable_with_timeout
      net eth0: visornic_enable_with_timeout success
      net eth0: visornic_open success

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 91 ++++++++++++++++++++++---
 1 file changed, 80 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 915c913..de05ac7 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -299,6 +299,8 @@ static int visor_thread_start(struct visor_thread_info *thrinfo,
 	init_completion(&thrinfo->has_stopped);
 	thrinfo->task = kthread_run(threadfn, thrcontext, name);
 	if (IS_ERR(thrinfo->task)) {
+		pr_debug("%s failed (%ld)\n",
+			 __func__, PTR_ERR(thrinfo->task));
 		thrinfo->id = 0;
 		return -EINVAL;
 	}
@@ -572,6 +574,8 @@ visornic_serverdown(struct visornic_devdata *devdata,
 		queue_work(visornic_serverdown_workqueue,
 			   &devdata->serverdown_completion);
 	} else if (devdata->server_change_state) {
+		dev_dbg(&devdata->dev->device, "%s changing state\n",
+			__func__);
 		return -EINVAL;
 	}
 	return 0;
@@ -708,6 +712,8 @@ visornic_disable_with_timeout(struct net_device *netdev, const int timeout)
 			break;
 		if (devdata->server_down || devdata->server_change_state) {
 			spin_unlock_irqrestore(&devdata->priv_lock, flags);
+			dev_dbg(&netdev->dev, "%s server went away\n",
+				__func__);
 			return -EIO;
 		}
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -821,8 +827,11 @@ visornic_enable_with_timeout(struct net_device *netdev, const int timeout)
 	 * gets a disable.
 	 */
 	i = init_rcv_bufs(netdev, devdata);
-	if (i < 0)
+	if (i < 0) {
+		dev_err(&netdev->dev,
+			"%s failed to init rcv bufs (%d)\n", __func__, i);
 		return i;
+	}
 
 	spin_lock_irqsave(&devdata->priv_lock, flags);
 	devdata->enabled = 1;
@@ -845,6 +854,8 @@ visornic_enable_with_timeout(struct net_device *netdev, const int timeout)
 			break;
 		if (devdata->server_down || devdata->server_change_state) {
 			spin_unlock_irqrestore(&devdata->priv_lock, flags);
+			dev_dbg(&netdev->dev, "%s server went away\n",
+				__func__);
 			return -EIO;
 		}
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -855,8 +866,10 @@ visornic_enable_with_timeout(struct net_device *netdev, const int timeout)
 
 	spin_unlock_irqrestore(&devdata->priv_lock, flags);
 
-	if (!devdata->enab_dis_acked)
+	if (!devdata->enab_dis_acked) {
+		dev_err(&netdev->dev, "%s missing ACK\n", __func__);
 		return -EIO;
+	}
 
 	/* find an open slot in the array to save off VisorNic references
 	 * for debug
@@ -968,6 +981,8 @@ visornic_xmit(struct sk_buff *skb, struct net_device *netdev)
 	    devdata->server_change_state) {
 		spin_unlock_irqrestore(&devdata->priv_lock, flags);
 		devdata->busy_cnt++;
+		dev_dbg(&netdev->dev,
+			"%s busy - queue stopped\n", __func__);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -986,6 +1001,9 @@ visornic_xmit(struct sk_buff *skb, struct net_device *netdev)
 	if (firstfraglen < ETH_HEADER_SIZE) {
 		spin_unlock_irqrestore(&devdata->priv_lock, flags);
 		devdata->busy_cnt++;
+		dev_err(&netdev->dev,
+			"%s busy - first frag too small (%d)\n",
+			__func__, firstfraglen);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -1025,6 +1043,9 @@ visornic_xmit(struct sk_buff *skb, struct net_device *netdev)
 		netif_stop_queue(netdev);
 		spin_unlock_irqrestore(&devdata->priv_lock, flags);
 		devdata->busy_cnt++;
+		dev_dbg(&netdev->dev,
+			"%s busy - waiting for iovm to catch up\n",
+			__func__);
 		return NETDEV_TX_BUSY;
 	}
 	if (devdata->queuefullmsg_logged)
@@ -1065,6 +1086,8 @@ visornic_xmit(struct sk_buff *skb, struct net_device *netdev)
 	if (cmdrsp->net.xmt.num_frags == -1) {
 		spin_unlock_irqrestore(&devdata->priv_lock, flags);
 		devdata->busy_cnt++;
+		dev_err(&netdev->dev,
+			"%s busy - copy frags failed\n", __func__);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -1073,6 +1096,8 @@ visornic_xmit(struct sk_buff *skb, struct net_device *netdev)
 		netif_stop_queue(netdev);
 		spin_unlock_irqrestore(&devdata->priv_lock, flags);
 		devdata->busy_cnt++;
+		dev_dbg(&netdev->dev,
+			"%s busy - signalinsert failed\n", __func__);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -1105,6 +1130,9 @@ visornic_xmit(struct sk_buff *skb, struct net_device *netdev)
 					   * netif_wake_queue() after lower
 					   * threshold
 					   */
+		dev_dbg(&netdev->dev,
+			"%s busy - invoking iovm flow control\n",
+			__func__);
 		devdata->flow_control_upper_hits++;
 	}
 	spin_unlock_irqrestore(&devdata->priv_lock, flags);
@@ -1211,6 +1239,8 @@ visornic_xmit_timeout(struct net_device *netdev)
 	/* Ensure that a ServerDown message hasn't been received */
 	if (!devdata->enabled ||
 	    (devdata->server_down && !devdata->server_change_state)) {
+		dev_dbg(&netdev->dev, "%s no processing\n",
+			__func__);
 		spin_unlock_irqrestore(&devdata->priv_lock, flags);
 		return;
 	}
@@ -1808,8 +1838,11 @@ static int visornic_probe(struct visor_device *dev)
 	u64 features;
 
 	netdev = alloc_etherdev(sizeof(struct visornic_devdata));
-	if (!netdev)
+	if (!netdev) {
+		dev_err(&dev->device,
+			"%s alloc_etherdev failed\n", __func__);
 		return -ENOMEM;
+	}
 
 	netdev->netdev_ops = &visornic_dev_ops;
 	netdev->watchdog_timeo = (5 * HZ);
@@ -1821,11 +1854,17 @@ static int visornic_probe(struct visor_device *dev)
 				  vnic.macaddr);
 	err = visorbus_read_channel(dev, channel_offset, netdev->dev_addr,
 				    ETH_ALEN);
-	if (err < 0)
+	if (err < 0) {
+		dev_err(&dev->device,
+			"%s failed to get mac addr from chan (%d)\n",
+			__func__, err);
 		goto cleanup_netdev;
+	}
 
 	devdata = devdata_initialize(netdev_priv(netdev), dev);
 	if (!devdata) {
+		dev_err(&dev->device,
+			"%s devdata_initialize failed\n", __func__);
 		err = -ENOMEM;
 		goto cleanup_netdev;
 	}
@@ -1842,8 +1881,12 @@ static int visornic_probe(struct visor_device *dev)
 				  vnic.num_rcv_bufs);
 	err = visorbus_read_channel(dev, channel_offset,
 				    &devdata->num_rcv_bufs, 4);
-	if (err)
+	if (err) {
+		dev_err(&dev->device,
+			"%s failed to get #rcv bufs from chan (%d)\n",
+			__func__, err);
 		goto cleanup_netdev;
+	}
 
 	devdata->rcvbuf = kzalloc(sizeof(struct sk_buff *) *
 				  devdata->num_rcv_bufs, GFP_KERNEL);
@@ -1884,38 +1927,58 @@ static int visornic_probe(struct visor_device *dev)
 	channel_offset = offsetof(struct spar_io_channel_protocol,
 				  vnic.mtu);
 	err = visorbus_read_channel(dev, channel_offset, &netdev->mtu, 4);
-	if (err)
+	if (err) {
+		dev_err(&dev->device,
+			"%s failed to get mtu from chan (%d)\n",
+			__func__, err);
 		goto cleanup_xmit_cmdrsp;
+	}
 
 	/* TODO: Setup Interrupt information */
 	/* Let's start our threads to get responses */
 	channel_offset = offsetof(struct spar_io_channel_protocol,
 				  channel_header.features);
 	err = visorbus_read_channel(dev, channel_offset, &features, 8);
-	if (err)
+	if (err) {
+		dev_err(&dev->device,
+			"%s failed to get features from chan (%d)\n",
+			__func__, err);
 		goto cleanup_xmit_cmdrsp;
+	}
 
 	features |= ULTRA_IO_CHANNEL_IS_POLLING;
 	err = visorbus_write_channel(dev, channel_offset, &features, 8);
-	if (err)
+	if (err) {
+		dev_err(&dev->device,
+			"%s failed to set features in chan (%d)\n",
+			__func__, err);
 		goto cleanup_xmit_cmdrsp;
+	}
 
 	devdata->thread_wait_ms = 2;
 	visor_thread_start(&devdata->threadinfo, process_incoming_rsps,
 			   devdata, "vnic_incoming");
 
 	err = register_netdev(netdev);
-	if (err)
+	if (err) {
+		dev_err(&dev->device,
+			"%s register_netdev failed (%d)\n", __func__, err);
 		goto cleanup_thread_stop;
+	}
 
 	/* create debgug/sysfs directories */
 	devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
 						      visornic_debugfs_dir);
 	if (!devdata->eth_debugfs_dir) {
+		dev_err(&dev->device,
+			"%s debugfs_create_dir %s failed\n",
+			__func__, netdev->name);
 		err = -ENOMEM;
 		goto cleanup_thread_stop;
 	}
 
+	dev_info(&dev->device, "%s success netdev=%s\n",
+		 __func__, netdev->name);
 	return 0;
 
 cleanup_thread_stop:
@@ -1963,8 +2026,10 @@ static void visornic_remove(struct visor_device *dev)
 {
 	struct visornic_devdata *devdata = dev_get_drvdata(&dev->device);
 
-	if (!devdata)
+	if (!devdata) {
+		dev_err(&dev->device, "%s no devdata\n", __func__);
 		return;
+	}
 	dev_set_drvdata(&dev->device, NULL);
 	host_side_disappeared(devdata);
 	kref_put(&devdata->kref, devdata_release);
@@ -2010,8 +2075,10 @@ static int visornic_resume(struct visor_device *dev,
 	unsigned long flags;
 
 	devdata = dev_get_drvdata(&dev->device);
-	if (!devdata)
+	if (!devdata) {
+		dev_err(&dev->device, "%s no devdata\n", __func__);
 		return -EINVAL;
+	}
 
 	netdev = devdata->netdev;
 
@@ -2039,6 +2106,8 @@ static int visornic_resume(struct visor_device *dev,
 		 */
 		send_enbdis(netdev, 1, devdata);
 	} else if (devdata->server_change_state) {
+		dev_err(&dev->device, "%s server_change_state\n",
+			__func__);
 		return -EIO;
 	}
 
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 07/13] staging: unisys: visornic: correct obvious double-allocation of workqueues

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Looks like an errant patch fitting caused us to redundantly allocate the
workqueues at both the beginning and end of visornic_init().  This was
corrected by removing the allocations at the beginning.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
Fixes: 68905a14e49c ('staging: unisys: Add s-Par visornic ethernet driver')
---
 drivers/staging/unisys/visornic/visornic_main.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index de05ac7..2ff7f2f 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -2127,18 +2127,6 @@ static int visornic_init(void)
 	struct dentry *ret;
 	int err = -ENOMEM;
 
-	/* create workqueue for serverdown completion */
-	visornic_serverdown_workqueue =
-		create_singlethread_workqueue("visornic_serverdown");
-	if (!visornic_serverdown_workqueue)
-		return -ENOMEM;
-
-	/* create workqueue for tx timeout reset */
-	visornic_timeout_reset_workqueue =
-		create_singlethread_workqueue("visornic_timeout_reset");
-	if (!visornic_timeout_reset_workqueue)
-		return -ENOMEM;
-
 	visornic_debugfs_dir = debugfs_create_dir("visornic", NULL);
 	if (!visornic_debugfs_dir)
 		return err;
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 08/13] staging: unisys: visornic: correctly clean up device on removal

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

visornic_remove() is called to logically detach the visornic driver from a
visorbus-supplied device, which can happen either just prior to a
visorbus-supplied device disappearing, or as a result of an rmmod of
visornic.  Prior to this patch, logic was missing to properly clean up for
this removal, which was fixed via the following changes:

* A going_away flag is now used to interlock between device destruction and
  workqueue operations, protected by priv_lock.  I.e., setting
  going_away=true under lock guarantees that no new work items can get
  queued to the work queues.  going_away=true also short-circuits other
  operations to enable device destruction to proceed.

* Missing clean-up operations for the workqueues, netdev, debugfs entries,
  and the worker thread were added.

* Memory referenced from the visornic private devdata struct is now freed
  as part of devdata destruction.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 65 ++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 2ff7f2f..ba46053 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -162,6 +162,7 @@ struct visornic_devdata {
 					  */
 	bool server_down;		 /* IOPART is down */
 	bool server_change_state;	 /* Processing SERVER_CHANGESTATE msg */
+	bool going_away;		 /* device is being torn down */
 	struct dentry *eth_debugfs_dir;
 	struct visor_thread_info threadinfo;
 	u64 interrupts_rcvd;
@@ -568,7 +569,17 @@ static int
 visornic_serverdown(struct visornic_devdata *devdata,
 		    visorbus_state_complete_func complete_func)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&devdata->priv_lock, flags);
 	if (!devdata->server_down && !devdata->server_change_state) {
+		if (devdata->going_away) {
+			spin_unlock_irqrestore(&devdata->priv_lock, flags);
+			dev_dbg(&devdata->dev->device,
+				"%s aborting because device removal pending\n",
+				__func__);
+			return -ENODEV;
+		}
 		devdata->server_change_state = true;
 		devdata->server_down_complete_func = complete_func;
 		queue_work(visornic_serverdown_workqueue,
@@ -576,8 +587,10 @@ visornic_serverdown(struct visornic_devdata *devdata,
 	} else if (devdata->server_change_state) {
 		dev_dbg(&devdata->dev->device, "%s changing state\n",
 			__func__);
+		spin_unlock_irqrestore(&devdata->priv_lock, flags);
 		return -EINVAL;
 	}
+	spin_unlock_irqrestore(&devdata->priv_lock, flags);
 	return 0;
 }
 
@@ -1236,6 +1249,14 @@ visornic_xmit_timeout(struct net_device *netdev)
 	unsigned long flags;
 
 	spin_lock_irqsave(&devdata->priv_lock, flags);
+	if (devdata->going_away) {
+		spin_unlock_irqrestore(&devdata->priv_lock, flags);
+		dev_dbg(&devdata->dev->device,
+			"%s aborting because device removal pending\n",
+			__func__);
+		return;
+	}
+
 	/* Ensure that a ServerDown message hasn't been received */
 	if (!devdata->enabled ||
 	    (devdata->server_down && !devdata->server_change_state)) {
@@ -1244,9 +1265,8 @@ visornic_xmit_timeout(struct net_device *netdev)
 		spin_unlock_irqrestore(&devdata->priv_lock, flags);
 		return;
 	}
-	spin_unlock_irqrestore(&devdata->priv_lock, flags);
-
 	queue_work(visornic_timeout_reset_workqueue, &devdata->timeout_reset);
+	spin_unlock_irqrestore(&devdata->priv_lock, flags);
 }
 
 /**
@@ -1614,6 +1634,9 @@ static void devdata_release(struct kref *mykref)
 	spin_lock(&lock_all_devices);
 	list_del(&devdata->list_all);
 	spin_unlock(&lock_all_devices);
+	kfree(devdata->rcvbuf);
+	kfree(devdata->cmdrsp_rcv);
+	kfree(devdata->xmit_cmdrsp);
 	kfree(devdata);
 }
 
@@ -2025,13 +2048,51 @@ static void host_side_disappeared(struct visornic_devdata *devdata)
 static void visornic_remove(struct visor_device *dev)
 {
 	struct visornic_devdata *devdata = dev_get_drvdata(&dev->device);
+	struct net_device *netdev;
+	unsigned long flags;
 
 	if (!devdata) {
 		dev_err(&dev->device, "%s no devdata\n", __func__);
 		return;
 	}
+	spin_lock_irqsave(&devdata->priv_lock, flags);
+	if (devdata->going_away) {
+		spin_unlock_irqrestore(&devdata->priv_lock, flags);
+		dev_err(&dev->device, "%s already being removed\n", __func__);
+		return;
+	}
+	devdata->going_away = true;
+	spin_unlock_irqrestore(&devdata->priv_lock, flags);
+	netdev = devdata->netdev;
+	if (!netdev) {
+		dev_err(&dev->device, "%s not net device\n", __func__);
+		return;
+	}
+
+	/* going_away prevents new items being added to the workqueues */
+	flush_workqueue(visornic_serverdown_workqueue);
+	flush_workqueue(visornic_timeout_reset_workqueue);
+
+	debugfs_remove_recursive(devdata->eth_debugfs_dir);
+
+	unregister_netdev(netdev);  /* this will call visornic_close() */
+
+	/* this had to wait until last because visornic_close() /
+	 * visornic_disable_with_timeout() polls waiting for state that is
+	 * only updated by the thread
+	 */
+	if (devdata->threadinfo.id) {
+		visor_thread_stop(&devdata->threadinfo);
+		if (devdata->threadinfo.id) {
+			dev_err(&dev->device, "%s cannot stop worker thread\n",
+				__func__);
+			return;
+		}
+	}
+
 	dev_set_drvdata(&dev->device, NULL);
 	host_side_disappeared(devdata);
+	free_netdev(netdev);
 	kref_put(&devdata->kref, devdata_release);
 }
 
-- 
2.1.4

[PATCH 09/13] staging: unisys: visornic: don't destroy global workqueues until devs destroyed

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

visornic_cleanup() was previously incorrectly destroying its global
workqueues prior to cleaning up the devices which used them.  This patch
corrects the order of these operations.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index ba46053..14977ad 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -2241,6 +2241,8 @@ cleanup_debugfs:
  */
 static void visornic_cleanup(void)
 {
+	visorbus_unregister_visor_driver(&visornic_driver);
+
 	if (visornic_serverdown_workqueue) {
 		flush_workqueue(visornic_serverdown_workqueue);
 		destroy_workqueue(visornic_serverdown_workqueue);
@@ -2251,7 +2253,6 @@ static void visornic_cleanup(void)
 	}
 	debugfs_remove_recursive(visornic_debugfs_dir);
 
-	visorbus_unregister_visor_driver(&visornic_driver);
 	kfree(dev_num_pool);
 	dev_num_pool = NULL;
 }
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 10/13] staging: unisys: visornic: delay start of worker thread until netdev created

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

This only makes sense, since the worker thread depends upon the netdev
existing.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 14977ad..07e89ad 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1978,15 +1978,11 @@ static int visornic_probe(struct visor_device *dev)
 		goto cleanup_xmit_cmdrsp;
 	}
 
-	devdata->thread_wait_ms = 2;
-	visor_thread_start(&devdata->threadinfo, process_incoming_rsps,
-			   devdata, "vnic_incoming");
-
 	err = register_netdev(netdev);
 	if (err) {
 		dev_err(&dev->device,
 			"%s register_netdev failed (%d)\n", __func__, err);
-		goto cleanup_thread_stop;
+		goto cleanup_xmit_cmdrsp;
 	}
 
 	/* create debgug/sysfs directories */
@@ -1997,16 +1993,17 @@ static int visornic_probe(struct visor_device *dev)
 			"%s debugfs_create_dir %s failed\n",
 			__func__, netdev->name);
 		err = -ENOMEM;
-		goto cleanup_thread_stop;
+		goto cleanup_xmit_cmdrsp;
 	}
 
+	devdata->thread_wait_ms = 2;
+	visor_thread_start(&devdata->threadinfo, process_incoming_rsps,
+			   devdata, "vnic_incoming");
+
 	dev_info(&dev->device, "%s success netdev=%s\n",
 		 __func__, netdev->name);
 	return 0;
 
-cleanup_thread_stop:
-	visor_thread_stop(&devdata->threadinfo);
-
 cleanup_xmit_cmdrsp:
 	kfree(devdata->xmit_cmdrsp);
 
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 11/13] staging: unisys: visornic: use preferred interface for setting netdev's parent

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

Just switch this line so it uses the correct function call.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 07e89ad..9b3c5d8 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1869,7 +1869,7 @@ static int visornic_probe(struct visor_device *dev)
 
 	netdev->netdev_ops = &visornic_dev_ops;
 	netdev->watchdog_timeo = (5 * HZ);
-	netdev->dev.parent = &dev->device;
+	SET_NETDEV_DEV(netdev, &dev->device);
 
 	/* Get MAC adddress from channel and read it into the device. */
 	netdev->addr_len = ETH_ALEN;
-- 
2.1.4

[PATCH 12/13] staging: unisys: visornic: prevent erroneous kfree of devdata pointer

[Benjamin Romer]

From: Tim Sell <Timothy.Sell@unisys.com>

A struct visornic_devdata for each visornic device is actually allocated as
part of alloc_etherdev(), here in visornic_probe():

    netdev = alloc_etherdev(sizeof(struct visornic_devdata));

But code in devdata_release() was treating devdata as a pointer that needed
to be kfree()d!  This was causing all sorts of weird behavior after doing
an rmmod of visornic, both because free_netdev() was actually freeing the
memory used for devdata, and because devdata wasn't pointing to
dynamically-allocated memory in the first place.

The kfree(devdata) and the kref that tracked devdata's usage have been
appropriately deleted.

Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 9b3c5d8..9350473 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -119,7 +119,6 @@ struct visornic_devdata {
 	struct visor_device *dev;
 	char name[99];
 	struct list_head list_all;   /* < link within list_all_devices list */
-	struct kref kref;
 	struct net_device *netdev;
 	struct net_device_stats net_stats;
 	atomic_t interrupt_rcvd;
@@ -1602,14 +1601,11 @@ devdata_initialize(struct visornic_devdata *devdata, struct visor_device *dev)
 	spin_unlock(&dev_num_pool_lock);
 	if (devnum == MAXDEVICES)
 		devnum = -1;
-	if (devnum < 0) {
-		kfree(devdata);
+	if (devnum < 0)
 		return NULL;
-	}
 	devdata->devnum = devnum;
 	devdata->dev = dev;
 	strncpy(devdata->name, dev_name(&dev->device), sizeof(devdata->name));
-	kref_init(&devdata->kref);
 	spin_lock(&lock_all_devices);
 	list_add_tail(&devdata->list_all, &list_all_devices);
 	spin_unlock(&lock_all_devices);
@@ -1617,17 +1613,14 @@ devdata_initialize(struct visornic_devdata *devdata, struct visor_device *dev)
 }
 
 /**
- *	devdata_release	- Frees up a devdata
- *	@mykref: kref to the devdata
+ *	devdata_release	- Frees up references in devdata
+ *	@devdata: struct to clean up
  *
- *	Frees up a devdata.
+ *	Frees up references in devdata.
  *	Returns void
  */
-static void devdata_release(struct kref *mykref)
+static void devdata_release(struct visornic_devdata *devdata)
 {
-	struct visornic_devdata *devdata =
-		container_of(mykref, struct visornic_devdata, kref);
-
 	spin_lock(&dev_num_pool_lock);
 	clear_bit(devdata->devnum, dev_num_pool);
 	spin_unlock(&dev_num_pool_lock);
@@ -1637,7 +1630,6 @@ static void devdata_release(struct kref *mykref)
 	kfree(devdata->rcvbuf);
 	kfree(devdata->cmdrsp_rcv);
 	kfree(devdata->xmit_cmdrsp);
-	kfree(devdata);
 }
 
 static const struct net_device_ops visornic_dev_ops = {
@@ -2089,8 +2081,8 @@ static void visornic_remove(struct visor_device *dev)
 
 	dev_set_drvdata(&dev->device, NULL);
 	host_side_disappeared(devdata);
+	devdata_release(devdata);
 	free_netdev(netdev);
-	kref_put(&devdata->kref, devdata_release);
 }
 
 /**
-- 
2.1.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 13/13] staging: unisys: Allow visorbus to autoload

[Benjamin Romer]

From: David Kershner <david.kershner@unisys.com>

We inadvertently remove the MODULE_DEVICE_TABLE line for visorbus,
this patch adds it back in.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
Fixes: d5b3f1dccee4 ('staging: unisys: move timskmod.h functionality')
---
 drivers/staging/unisys/visorbus/visorchipset.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index d47ec24..4be9318 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -2382,6 +2382,9 @@ static struct acpi_driver unisys_acpi_driver = {
 		.remove = visorchipset_exit,
 		},
 };
+
+MODULE_DEVICE_TABLE(acpi, unisys_device_ids);
+
 static __init uint32_t visorutil_spar_detect(void)
 {
 	unsigned int eax, ebx, ecx, edx;
-- 
2.1.4

Re: [PATCH 07/13] staging: unisys: visornic: correct obvious double-allocation of workqueues

[Dan Carpenter]

On Thu, Jul 09, 2015 at 01:27:47PM -0400, Benjamin Romer wrote:
> From: Tim Sell <Timothy.Sell@unisys.com>
> 
> Looks like an errant patch fitting caused us to redundantly allocate the
> workqueues at both the beginning and end of visornic_init().  This was
> corrected by removing the allocations at the beginning.
> 
> Signed-off-by: Tim Sell <Timothy.Sell@unisys.com>
> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>

Btw, we have a Fixes tag, which normally goes right before the Sign
offs.  Don't resend.  I was just curious how we missed this in review.

Fixes: 68905a14e49c ('staging: unisys: Add s-Par visornic ethernet driver')

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/13] staging: unisys: Allow visorbus to autoload

[Dan Carpenter]

On Thu, Jul 09, 2015 at 01:27:53PM -0400, Benjamin Romer wrote:
> From: David Kershner <david.kershner@unisys.com>
> 
> We inadvertently remove the MODULE_DEVICE_TABLE line for visorbus,
> this patch adds it back in.
> 
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>

Fixes: d5b3f1dccee4 ('staging: unisys: move timskmod.h functionality')

This was part of a 141 patch series and it wasn't reviewed according to
our normal standards because who wants to redo 141 patches...  :/
Hopefully, there won't be too many more series like that.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel