[PATCH 0/8] staging: unisys: visornic/visorbus fixes

[PATCH 0/8] staging: unisys: visornic/visorbus fixes

[Benjamin Romer]

This patch series addresses several issues found during review,
testing, and by the kernel test bot.

Benjamin Romer (2):
  staging: unisys: stop device registration before visorbus registration
  staging: unisys: visornic: handle error return from device
    registration

David Kershner (6):
  staging: unisys: unregister netdev when create debugfs fails
  staging: unisys: remove devdata->name use netdev->name
  staging: unisys: get rid of devnum pool and dev num
  staging: unisys: git rid of list of devices
  staging: unisys: visornic: Fix receive bytes statistics
  staging: unisys: visorbus: Unregister driver on error

 drivers/staging/unisys/visorbus/visorbus_main.c |  5 +++
 drivers/staging/unisys/visornic/visornic_main.c | 60 +++++--------------------
 2 files changed, 16 insertions(+), 49 deletions(-)

-- 
2.1.4

[PATCH 1/8] staging: unisys: unregister netdev when create debugfs fails

[Benjamin Romer]

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

Noticed we were not unregistering the netdevice if we failed to
create the debugfs entries. This patch fixes that problem.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 8c9da7e..bacf1af 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1924,13 +1924,16 @@ static int visornic_probe(struct visor_device *dev)
 			"%s debugfs_create_dir %s failed\n",
 			__func__, netdev->name);
 		err = -ENOMEM;
-		goto cleanup_xmit_cmdrsp;
+		goto cleanup_register_netdev;
 	}
 
 	dev_info(&dev->device, "%s success netdev=%s\n",
 		 __func__, netdev->name);
 	return 0;
 
+cleanup_register_netdev:
+	unregister_netdev(netdev);
+
 cleanup_napi_add:
 	del_timer_sync(&devdata->irq_poll_timer);
 	netif_napi_del(&devdata->napi);
-- 
2.1.4

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

[PATCH 2/8] staging: unisys: remove devdata->name use netdev->name

[Benjamin Romer]

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

The net device already has a name, use that instead

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index bacf1af..6ec0cfd 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -119,7 +119,6 @@ struct visornic_devdata {
 					 * IOPART
 					 */
 	struct visor_device *dev;
-	char name[99];
 	struct list_head list_all;   /* < link within list_all_devices list */
 	struct net_device *netdev;
 	struct net_device_stats net_stats;
@@ -1388,7 +1387,6 @@ devdata_initialize(struct visornic_devdata *devdata, struct visor_device *dev)
 		return NULL;
 	devdata->devnum = devnum;
 	devdata->dev = dev;
-	strncpy(devdata->name, dev_name(&dev->device), sizeof(devdata->name));
 	spin_lock(&lock_all_devices);
 	list_add_tail(&devdata->list_all, &list_all_devices);
 	spin_unlock(&lock_all_devices);
@@ -1964,7 +1962,6 @@ static void host_side_disappeared(struct visornic_devdata *devdata)
 	unsigned long flags;
 
 	spin_lock_irqsave(&devdata->priv_lock, flags);
-	sprintf(devdata->name, "<dev#%d-history>", devdata->devnum);
 	devdata->dev = NULL;   /* indicate device destroyed */
 	spin_unlock_irqrestore(&devdata->priv_lock, flags);
 }
-- 
2.1.4

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

[PATCH 3/8] staging: unisys: get rid of devnum pool and dev num

[Benjamin Romer]

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

devnum pool and devnum are no longer needed. Just
get rid of them.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 26 -------------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 6ec0cfd..2836f45 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -37,9 +37,6 @@
  */
 #define MAX_BUF 163840
 
-static spinlock_t dev_num_pool_lock;
-static void *dev_num_pool;	/**< pool to grab device numbers from */
-
 static int visornic_probe(struct visor_device *dev);
 static void visornic_remove(struct visor_device *dev);
 static int visornic_pause(struct visor_device *dev,
@@ -113,7 +110,6 @@ struct chanstat {
 };
 
 struct visornic_devdata {
-	int devnum;
 	unsigned short enabled;		/* 0 disabled 1 enabled to receive */
 	unsigned short enab_dis_acked;	/* NET_RCV_ENABLE/DISABLE acked by
 					 * IOPART
@@ -1372,20 +1368,9 @@ visornic_rx(struct uiscmdrsp *cmdrsp)
 static struct visornic_devdata *
 devdata_initialize(struct visornic_devdata *devdata, struct visor_device *dev)
 {
-	int devnum = -1;
-
 	if (!devdata)
 		return NULL;
 	memset(devdata, '\0', sizeof(struct visornic_devdata));
-	spin_lock(&dev_num_pool_lock);
-	devnum = find_first_zero_bit(dev_num_pool, MAXDEVICES);
-	set_bit(devnum, dev_num_pool);
-	spin_unlock(&dev_num_pool_lock);
-	if (devnum == MAXDEVICES)
-		devnum = -1;
-	if (devnum < 0)
-		return NULL;
-	devdata->devnum = devnum;
 	devdata->dev = dev;
 	spin_lock(&lock_all_devices);
 	list_add_tail(&devdata->list_all, &list_all_devices);
@@ -1402,9 +1387,6 @@ devdata_initialize(struct visornic_devdata *devdata, struct visor_device *dev)
  */
 static void devdata_release(struct visornic_devdata *devdata)
 {
-	spin_lock(&dev_num_pool_lock);
-	clear_bit(devdata->devnum, dev_num_pool);
-	spin_unlock(&dev_num_pool_lock);
 	spin_lock(&lock_all_devices);
 	list_del(&devdata->list_all);
 	spin_unlock(&lock_all_devices);
@@ -2123,11 +2105,6 @@ static int visornic_init(void)
 	if (!visornic_timeout_reset_workqueue)
 		goto cleanup_workqueue;
 
-	spin_lock_init(&dev_num_pool_lock);
-	dev_num_pool = kzalloc(BITS_TO_LONGS(MAXDEVICES), GFP_KERNEL);
-	if (!dev_num_pool)
-		goto cleanup_workqueue;
-
 	visorbus_register_visor_driver(&visornic_driver);
 	return 0;
 
@@ -2156,9 +2133,6 @@ static void visornic_cleanup(void)
 		destroy_workqueue(visornic_timeout_reset_workqueue);
 	}
 	debugfs_remove_recursive(visornic_debugfs_dir);
-
-	kfree(dev_num_pool);
-	dev_num_pool = NULL;
 }
 
 module_init(visornic_init);
-- 
2.1.4

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

[PATCH 4/8] staging: unisys: git rid of list of devices

[Benjamin Romer]

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

We don't need the list of devices, we can loop through the one
provided by the network api and filter on ours.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 2836f45..a0594b1 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -115,7 +115,6 @@ struct visornic_devdata {
 					 * IOPART
 					 */
 	struct visor_device *dev;
-	struct list_head list_all;   /* < link within list_all_devices list */
 	struct net_device *netdev;
 	struct net_device_stats net_stats;
 	atomic_t interrupt_rcvd;
@@ -196,12 +195,6 @@ struct visornic_devdata {
 	struct uiscmdrsp cmdrsp[SIZEOF_CMDRSP];
 };
 
-
-/* List of all visornic_devdata structs,
- * linked via the list_all member
- */
-static LIST_HEAD(list_all_devices);
-static DEFINE_SPINLOCK(lock_all_devices);
 static int visornic_poll(struct napi_struct *napi, int budget);
 static void poll_for_irq(unsigned long v);
 
@@ -1372,9 +1365,6 @@ devdata_initialize(struct visornic_devdata *devdata, struct visor_device *dev)
 		return NULL;
 	memset(devdata, '\0', sizeof(struct visornic_devdata));
 	devdata->dev = dev;
-	spin_lock(&lock_all_devices);
-	list_add_tail(&devdata->list_all, &list_all_devices);
-	spin_unlock(&lock_all_devices);
 	return devdata;
 }
 
@@ -1387,9 +1377,6 @@ devdata_initialize(struct visornic_devdata *devdata, struct visor_device *dev)
  */
 static void devdata_release(struct visornic_devdata *devdata)
 {
-	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);
-- 
2.1.4

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

[PATCH 5/8] staging: unisys: visornic: Fix receive bytes statistics

[Benjamin Romer]

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

The receive byte statistics was wrong in /proc/net/dev.

Move the collection of statistics after the proper amount
of bytes has been calculated and make sure you add it to
rx_bytes instead of just replacing it.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visornic/visornic_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index a0594b1..dcd7806 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -1177,16 +1177,16 @@ visornic_rx(struct uiscmdrsp *cmdrsp)
 	spin_lock_irqsave(&devdata->priv_lock, flags);
 	atomic_dec(&devdata->num_rcvbuf_in_iovm);
 
-	/* update rcv stats - call it with priv_lock held */
-	devdata->net_stats.rx_packets++;
-	devdata->net_stats.rx_bytes = skb->len;
-
 	/* set length to how much was ACTUALLY received -
 	 * NOTE: rcv_done_len includes actual length of data rcvd
 	 * including ethhdr
 	 */
 	skb->len = cmdrsp->net.rcv.rcv_done_len;
 
+	/* update rcv stats - call it with priv_lock held */
+	devdata->net_stats.rx_packets++;
+	devdata->net_stats.rx_bytes += skb->len;
+
 	/* test enabled while holding lock */
 	if (!(devdata->enabled && devdata->enab_dis_acked)) {
 		/* don't process it unless we're in enable mode and until
-- 
2.1.4

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

[PATCH 6/8] staging: unisys: visorbus: Unregister driver on error

[Benjamin Romer]

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

If there is an error in registering driver attributes, unregister
the driver as well.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 2309f5f..7905ea9 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -885,6 +885,8 @@ int visorbus_register_visor_driver(struct visor_driver *drv)
 	if (rc < 0)
 		return rc;
 	rc = register_driver_attributes(drv);
+	if (rc < 0)
+		driver_unregister(&drv->driver);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(visorbus_register_visor_driver);
-- 
2.1.4

[PATCH 7/8] staging: unisys: stop device registration before visorbus registration

[Benjamin Romer]

In cases where visorbus is compiled directly into the kernel, if
visorbus registration fails for any reason, it is still possible for
other drivers to call visorbus_register_visor_driver(), which could
cause an oops. Prevent this by returning an error code when the bus
hasn't been registered.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 7905ea9..ad2b1ac 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -863,6 +863,9 @@ int visorbus_register_visor_driver(struct visor_driver *drv)
 {
 	int rc = 0;
 
+	if (!visorbus_type.p)
+		return -ENODEV; /*can't register on a nonexistent bus*/
+
 	drv->driver.name = drv->name;
 	drv->driver.bus = &visorbus_type;
 	drv->driver.probe = visordriver_probe_device;
-- 
2.1.4

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

[PATCH 8/8] staging: unisys: visornic: handle error return from device registration

[Benjamin Romer]

There is no code to handle an error return in visornic, when it tries to
register with visorbus. This patch handles an error return from
visorbus_register_visor_driver() by dropping out of initialization.

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

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index dcd7806..85c9fec 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -2092,8 +2092,9 @@ static int visornic_init(void)
 	if (!visornic_timeout_reset_workqueue)
 		goto cleanup_workqueue;
 
-	visorbus_register_visor_driver(&visornic_driver);
-	return 0;
+	err = visorbus_register_visor_driver(&visornic_driver);
+	if (!err)
+		return 0;
 
 cleanup_workqueue:
 	if (visornic_timeout_reset_workqueue) {
-- 
2.1.4

Re: [PATCH 7/8] staging: unisys: stop device registration before visorbus registration

[Sudip Mukherjee]

On Tue, Aug 18, 2015 at 03:14:02PM -0400, Benjamin Romer wrote:
> In cases where visorbus is compiled directly into the kernel, if
> visorbus registration fails for any reason, it is still possible for
> other drivers to call visorbus_register_visor_driver(), which could
> cause an oops. Prevent this by returning an error code when the bus
> hasn't been registered.
> 
> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
> index 7905ea9..ad2b1ac 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -863,6 +863,9 @@ int visorbus_register_visor_driver(struct visor_driver *drv)
>  {
>  	int rc = 0;
>  
> +	if (!visorbus_type.p)
> +		return -ENODEV; /*can't register on a nonexistent bus*/
> +
IIRC, Greg once told that we should not be working with the internal
data structures of struct bus_type.

regards
sudip

Re: [PATCH 7/8] staging: unisys: stop device registration before visorbus registration

[Jes Sorensen]

Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
> On Tue, Aug 18, 2015 at 03:14:02PM -0400, Benjamin Romer wrote:
>> In cases where visorbus is compiled directly into the kernel, if
>> visorbus registration fails for any reason, it is still possible for
>> other drivers to call visorbus_register_visor_driver(), which could
>> cause an oops. Prevent this by returning an error code when the bus
>> hasn't been registered.
>> 
>> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
>> ---
>>  drivers/staging/unisys/visorbus/visorbus_main.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
>> index 7905ea9..ad2b1ac 100644
>> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
>> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
>> @@ -863,6 +863,9 @@ int visorbus_register_visor_driver(struct visor_driver *drv)
>>  {
>>  	int rc = 0;
>>  
>> +	if (!visorbus_type.p)
>> +		return -ENODEV; /*can't register on a nonexistent bus*/
>> +
> IIRC, Greg once told that we should not be working with the internal
> data structures of struct bus_type.

If you looked at the code you would have noticed this is in fact the bus
driver, and visorbus_type is defined in this file. I guess we could tell
visorbus_main.c to not touch visorbus_type by deleting the file
completely .....

Re: [PATCH 7/8] staging: unisys: stop device registration before visorbus registration

[Sudip Mukherjee]

On Tue, Aug 25, 2015 at 08:33:55AM -0400, Jes Sorensen wrote:
> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
> > On Tue, Aug 18, 2015 at 03:14:02PM -0400, Benjamin Romer wrote:
> >> In cases where visorbus is compiled directly into the kernel, if
> >> visorbus registration fails for any reason, it is still possible for
> >> other drivers to call visorbus_register_visor_driver(), which could
> >> cause an oops. Prevent this by returning an error code when the bus
> >> hasn't been registered.
> >> 
> >> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> >> ---
> >>  drivers/staging/unisys/visorbus/visorbus_main.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
> >> index 7905ea9..ad2b1ac 100644
> >> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> >> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> >> @@ -863,6 +863,9 @@ int visorbus_register_visor_driver(struct visor_driver *drv)
> >>  {
> >>  	int rc = 0;
> >>  
> >> +	if (!visorbus_type.p)
> >> +		return -ENODEV; /*can't register on a nonexistent bus*/
> >> +
> > IIRC, Greg once told that we should not be working with the internal
> > data structures of struct bus_type.
> 
> If you looked at the code you would have noticed this is in fact the bus
> driver, and visorbus_type is defined in this file. I guess we could tell
> visorbus_main.c to not touch visorbus_type by deleting the file
> completely .....
Yes, this is the bus driver, struct bus_type. Maybe the following might
be a better approach where the struct bus_type internals are not touched
yet it will server the purpose.

regards
sudip

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 2309f5f..32500be 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -31,6 +31,7 @@ static int visorbus_debug;
 static int visorbus_forcematch;
 static int visorbus_forcenomatch;
 static int visorbus_debugref;
+static bool bus_registered;
 #define SERIALLOOPBACKCHANADDR (100 * 1024 * 1024)
 
 #define CURRENT_FILE_PC VISOR_BUS_PC_visorbus_main_c
@@ -40,6 +41,7 @@ static int visorbus_debugref;
 static int visorbus_uevent(struct device *xdev, struct kobj_uevent_env *env);
 static int visorbus_match(struct device *xdev, struct device_driver *xdrv);
 static void fix_vbus_dev_info(struct visor_device *visordev);
+static bool is_visorbus_registered(void);
 
 /*  BUS type attributes
  *
@@ -863,6 +865,9 @@ int visorbus_register_visor_driver(struct visor_driver *drv)
 {
 	int rc = 0;
 
+	if(!is_visorbus_registered())
+		return -ENODEV; /*can't register on a nonexistent bus*/
+
 	drv->driver.name = drv->name;
 	drv->driver.bus = &visorbus_type;
 	drv->driver.probe = visordriver_probe_device;
@@ -1263,7 +1268,17 @@ create_bus_type(void)
 	int rc = 0;
 
 	rc = bus_register(&visorbus_type);
-	return rc;
+
+	if (rc)
+		return rc;
+	bus_registered = true;
+	return 0;
+}
+
+static bool
+is_visorbus_registered(void)
+{
+	return bus_registered;
 }
 
 /** Remove the one-and-only one instance of the visor bus type (visorbus_type).

Re: [PATCH 7/8] staging: unisys: stop device registration before visorbus registration

[Jes Sorensen]

Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
> On Tue, Aug 25, 2015 at 08:33:55AM -0400, Jes Sorensen wrote:
>> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>> > On Tue, Aug 18, 2015 at 03:14:02PM -0400, Benjamin Romer wrote:
>> >> In cases where visorbus is compiled directly into the kernel, if
>> >> visorbus registration fails for any reason, it is still possible for
>> >> other drivers to call visorbus_register_visor_driver(), which could
>> >> cause an oops. Prevent this by returning an error code when the bus
>> >> hasn't been registered.
>> >> 
>> >> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
>> >> ---
>> >>  drivers/staging/unisys/visorbus/visorbus_main.c | 3 +++
>> >>  1 file changed, 3 insertions(+)
>> >> 
>> >> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
>> >> index 7905ea9..ad2b1ac 100644
>> >> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
>> >> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
>> >> @@ -863,6 +863,9 @@ int visorbus_register_visor_driver(struct visor_driver *drv)
>> >>  {
>> >>  	int rc = 0;
>> >>  
>> >> +	if (!visorbus_type.p)
>> >> +		return -ENODEV; /*can't register on a nonexistent bus*/
>> >> +
>> > IIRC, Greg once told that we should not be working with the internal
>> > data structures of struct bus_type.
>> 
>> If you looked at the code you would have noticed this is in fact the bus
>> driver, and visorbus_type is defined in this file. I guess we could tell
>> visorbus_main.c to not touch visorbus_type by deleting the file
>> completely .....
> Yes, this is the bus driver, struct bus_type. Maybe the following might
> be a better approach where the struct bus_type internals are not touched
> yet it will server the purpose.

Replacing it with a global variable? Ehm sorry, but no thanks!

NACK

Jes

Re: [PATCH 7/8] staging: unisys: stop device registration before visorbus registration

[Greg KH]

On Wed, Aug 26, 2015 at 07:15:26AM -0400, Jes Sorensen wrote:
> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
> > On Tue, Aug 25, 2015 at 08:33:55AM -0400, Jes Sorensen wrote:
> >> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
> >> > On Tue, Aug 18, 2015 at 03:14:02PM -0400, Benjamin Romer wrote:
> >> >> In cases where visorbus is compiled directly into the kernel, if
> >> >> visorbus registration fails for any reason, it is still possible for
> >> >> other drivers to call visorbus_register_visor_driver(), which could
> >> >> cause an oops. Prevent this by returning an error code when the bus
> >> >> hasn't been registered.
> >> >> 
> >> >> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> >> >> ---
> >> >>  drivers/staging/unisys/visorbus/visorbus_main.c | 3 +++
> >> >>  1 file changed, 3 insertions(+)
> >> >> 
> >> >> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
> >> >> index 7905ea9..ad2b1ac 100644
> >> >> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> >> >> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> >> >> @@ -863,6 +863,9 @@ int visorbus_register_visor_driver(struct visor_driver *drv)
> >> >>  {
> >> >>  	int rc = 0;
> >> >>  
> >> >> +	if (!visorbus_type.p)
> >> >> +		return -ENODEV; /*can't register on a nonexistent bus*/
> >> >> +
> >> > IIRC, Greg once told that we should not be working with the internal
> >> > data structures of struct bus_type.
> >> 
> >> If you looked at the code you would have noticed this is in fact the bus
> >> driver, and visorbus_type is defined in this file. I guess we could tell
> >> visorbus_main.c to not touch visorbus_type by deleting the file
> >> completely .....
> > Yes, this is the bus driver, struct bus_type. Maybe the following might
> > be a better approach where the struct bus_type internals are not touched
> > yet it will server the purpose.
> 
> Replacing it with a global variable? Ehm sorry, but no thanks!
> 
> NACK

It should be static to the bus code.  Don't go poking at any .p
structures in the driver core, that's not ok either, I can't take this
as-is.  You should know if you can properly register a device/driver or
not.

thanks,

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

Re: [PATCH 0/8] staging: unisys: visornic/visorbus fixes

[Greg KH]

On Tue, Aug 18, 2015 at 03:13:55PM -0400, Benjamin Romer wrote:
> This patch series addresses several issues found during review,
> testing, and by the kernel test bot.

These aren't all "fixes" so I can't just add them to the 4.3-final
release, but some look like they should go there.  So please split this
into two different series and resend, one for 4.3-final and one for
4.4-rc1.

thanks,

greg k-h

Re: [PATCH 0/8] staging: unisys: visornic/visorbus fixes

[Ben Romer]

On 09/02/2015 08:40 PM, Greg KH wrote:
> These aren't all "fixes" so I can't just add them to the 4.3-final
> release, but some look like they should go there.  So please split this
> into two different series and resend, one for 4.3-final and one for
> 4.4-rc1.

Greg,

We've been running and testing with all of these patches for a week or 
two now, and my team and I all feel that the entire set is safe to 
include in the new release. I'm not sure what your criteria are for 
inclusion in 4.3, so if you could let me know what those are, I'd be 
happy to split them up and send two sets, of course with the per-patch 
comments addressed. :)

Thanks!

-- Ben

Re: [PATCH 0/8] staging: unisys: visornic/visorbus fixes

[Sudip Mukherjee]

On Fri, Sep 04, 2015 at 09:20:23AM -0400, Ben Romer wrote:
> On 09/02/2015 08:40 PM, Greg KH wrote:
> >These aren't all "fixes" so I can't just add them to the 4.3-final
> >release, but some look like they should go there.  So please split this
> >into two different series and resend, one for 4.3-final and one for
> >4.4-rc1.
> 
> Greg,
> 
> We've been running and testing with all of these patches for a week
> or two now, and my team and I all feel that the entire set is safe
> to include in the new release. I'm not sure what your criteria are
> for inclusion in 4.3, so if you could let me know what those are,
> I'd be happy to split them up and send two sets, of course with the
> per-patch comments addressed. :)
Any new developments can only go in rc1 release. 4.3 merge window is
already open so new developments cannot be added to 4.3-rc1. New codes
has to go to 4.4-rc1.
4.3-rc2 - 4.3-rc7 (or 8) can only have fixes.

Greg please correct me if I am wrong. I should not have replied here but
this is a good opportunity to test my understanding of the release
cycle. :)

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

Re: [PATCH 0/8] staging: unisys: visornic/visorbus fixes

[Ben Romer]

On 09/04/2015 09:41 AM, Sudip Mukherjee wrote:
> Any new developments can only go in rc1 release. 4.3 merge window is
> already open so new developments cannot be added to 4.3-rc1. New codes
> has to go to 4.4-rc1.
> 4.3-rc2 - 4.3-rc7 (or 8) can only have fixes.
I don't think any of the patches in this set specifically were adding 
new features, though I suppose the patches to remove list_all, 
devnum_pool, and devnum could be seen as not being *bug* fixes. It 
depends on if eliminating redundant code counts or not.

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

Re: [PATCH 0/8] staging: unisys: visornic/visorbus fixes

[Greg KH]

On Fri, Sep 04, 2015 at 09:58:10AM -0400, Ben Romer wrote:
> On 09/04/2015 09:41 AM, Sudip Mukherjee wrote:
> >Any new developments can only go in rc1 release. 4.3 merge window is
> >already open so new developments cannot be added to 4.3-rc1. New codes
> >has to go to 4.4-rc1.
> >4.3-rc2 - 4.3-rc7 (or 8) can only have fixes.
> I don't think any of the patches in this set specifically were adding new
> features, though I suppose the patches to remove list_all, devnum_pool, and
> devnum could be seen as not being *bug* fixes. It depends on if eliminating
> redundant code counts or not.

It does not.  That's not a "bug" or "regression" that is needing to be
fixed as it is causing a problem for people.  It's a "cleanup" and that
type of patch can only be added in -rc1.

hope this helps,

greg k-h

Re: [PATCH 0/8] staging: unisys: visornic/visorbus fixes

[Sudip Mukherjee]

On Fri, Sep 04, 2015 at 09:58:10AM -0400, Ben Romer wrote:
> On 09/04/2015 09:41 AM, Sudip Mukherjee wrote:
> >Any new developments can only go in rc1 release. 4.3 merge window is
> >already open so new developments cannot be added to 4.3-rc1. New codes
> >has to go to 4.4-rc1.
> >4.3-rc2 - 4.3-rc7 (or 8) can only have fixes.
> I don't think any of the patches in this set specifically were
> adding new features, though I suppose the patches to remove
> list_all, devnum_pool, and devnum could be seen as not being *bug*
> fixes. It depends on if eliminating redundant code counts or not.
Eliminating redundant codes is considered as development and not bug
fix and should go to 4.4-rc1. The fixes like the one 0 day bot pointed
out should be for 4.3.

regards
sudip