[PATCH 1/8] pwcd_usb: fix error handling in probe

[PATCH 1/8] pwcd_usb: fix error handling in probe

[Oliver Neukum]

If you support only one device, you need to roll back
back your counter in all cases if probe() fails.
That cannot be left to usb_pcwd_delete() because
that must be called conditionally.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/watchdog/pcwd_usb.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/pcwd_usb.c b/drivers/watchdog/pcwd_usb.c
index 8202f0a6b093..3a22291d633b 100644
--- a/drivers/watchdog/pcwd_usb.c
+++ b/drivers/watchdog/pcwd_usb.c
@@ -606,7 +606,7 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 	struct usb_endpoint_descriptor *endpoint;
 	struct usb_pcwd_private *usb_pcwd = NULL;
 	int pipe;
-	int retval = -ENOMEM;
+	int retval = -ENODEV;
 	int got_fw_rev;
 	unsigned char fw_rev_major, fw_rev_minor;
 	char fw_ver_str[20];
@@ -615,7 +615,7 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 	cards_found++;
 	if (cards_found > 1) {
 		pr_err("This driver only supports 1 device\n");
-		return -ENODEV;
+		goto error_count;
 	}
 
 	/* get the active interface descriptor */
@@ -624,11 +624,12 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 	/* check out that we have a HID device */
 	if (!(iface_desc->desc.bInterfaceClass == USB_CLASS_HID)) {
 		pr_err("The device isn't a Human Interface Device\n");
-		return -ENODEV;
+		goto error_count;
 	}
 
 	if (iface_desc->desc.bNumEndpoints < 1)
-		return -ENODEV;
+		goto error_count;
+	
 
 	/* check out the endpoint: it has to be Interrupt & IN */
 	endpoint = &iface_desc->endpoint[0].desc;
@@ -636,16 +637,17 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 	if (!usb_endpoint_is_int_in(endpoint)) {
 		/* we didn't find a Interrupt endpoint with direction IN */
 		pr_err("Couldn't find an INTR & IN endpoint\n");
-		return -ENODEV;
+		goto error_count;
 	}
 
 	/* get a handle to the interrupt data pipe */
 	pipe = usb_rcvintpipe(udev, endpoint->bEndpointAddress);
 
 	/* allocate memory for our device and initialize it */
+	retval = -ENOMEM;
 	usb_pcwd = kzalloc(sizeof(struct usb_pcwd_private), GFP_KERNEL);
 	if (usb_pcwd == NULL)
-		goto error;
+		goto error_count;
 
 	usb_pcwd_device = usb_pcwd;
 
@@ -661,13 +663,13 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 					GFP_KERNEL, &usb_pcwd->intr_dma);
 	if (!usb_pcwd->intr_buffer) {
 		pr_err("Out of memory\n");
-		goto error;
+		goto error_delete;
 	}
 
 	/* allocate the urb's */
 	usb_pcwd->intr_urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!usb_pcwd->intr_urb)
-		goto error;
+		goto error_delete;
 
 	/* initialise the intr urb's */
 	usb_fill_int_urb(usb_pcwd->intr_urb, udev, pipe,
@@ -680,7 +682,7 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 	if (usb_submit_urb(usb_pcwd->intr_urb, GFP_KERNEL)) {
 		pr_err("Problem registering interrupt URB\n");
 		retval = -EIO; /* failure */
-		goto error;
+		goto error_delete;
 	}
 
 	/* The device exists and can be communicated with */
@@ -723,7 +725,7 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 	retval = register_reboot_notifier(&usb_pcwd_notifier);
 	if (retval != 0) {
 		pr_err("cannot register reboot notifier (err=%d)\n", retval);
-		goto error;
+		goto error_delete;
 	}
 
 	retval = misc_register(&usb_pcwd_temperature_miscdev);
@@ -752,10 +754,12 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 	misc_deregister(&usb_pcwd_temperature_miscdev);
 err_out_unregister_reboot:
 	unregister_reboot_notifier(&usb_pcwd_notifier);
-error:
+error_delete:
 	if (usb_pcwd)
 		usb_pcwd_delete(usb_pcwd);
 	usb_pcwd_device = NULL;
+error_count:
+	cards_found--;
 	return retval;
 }
 
-- 
2.40.0

[PATCH 2/8] pcwd_usb: stop open race disconnect

[Oliver Neukum]

There is a mutex already taken in disconnect.
And it even has the correct comment.
It just has to be taken in open and disconnect, too,
to prevent the race.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/watchdog/pcwd_usb.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/pcwd_usb.c b/drivers/watchdog/pcwd_usb.c
index 3a22291d633b..d5d68a529620 100644
--- a/drivers/watchdog/pcwd_usb.c
+++ b/drivers/watchdog/pcwd_usb.c
@@ -476,14 +476,21 @@ static long usb_pcwd_ioctl(struct file *file, unsigned int cmd,
 
 static int usb_pcwd_open(struct inode *inode, struct file *file)
 {
+	int retval = -EBUSY;
+
+	mutex_lock(&disconnect_mutex);
 	/* /dev/watchdog can only be opened once */
 	if (test_and_set_bit(0, &is_active))
-		return -EBUSY;
+		goto error;
 
 	/* Activate */
 	usb_pcwd_start(usb_pcwd_device);
 	usb_pcwd_keepalive(usb_pcwd_device);
-	return stream_open(inode, file);
+	retval = stream_open(inode, file);
+
+error:
+	mutex_unlock(&disconnect_mutex);
+	return retval;
 }
 
 static int usb_pcwd_release(struct inode *inode, struct file *file)
@@ -522,7 +529,13 @@ static ssize_t usb_pcwd_temperature_read(struct file *file, char __user *data,
 
 static int usb_pcwd_temperature_open(struct inode *inode, struct file *file)
 {
-	return stream_open(inode, file);
+	int retval;
+
+	mutex_lock(&disconnect_mutex);
+	retval = stream_open(inode, file);
+	mutex_unlock(&disconnect_mutex);
+
+	return retval;
 }
 
 static int usb_pcwd_temperature_release(struct inode *inode, struct file *file)
-- 
2.40.0

[PATCH 3/8] pcwd_usb: usb_pcwd_open handle and return errors

[Oliver Neukum]

Operations on the bus can suffer an IO error.
Handle it undoing partial operations and return
it to user space

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/watchdog/pcwd_usb.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/pcwd_usb.c b/drivers/watchdog/pcwd_usb.c
index d5d68a529620..ed3be8926a15 100644
--- a/drivers/watchdog/pcwd_usb.c
+++ b/drivers/watchdog/pcwd_usb.c
@@ -273,7 +273,7 @@ static int usb_pcwd_start(struct usb_pcwd_private *usb_pcwd)
 
 	if ((retval == 0) || (lsb == 0)) {
 		pr_err("Card did not acknowledge enable attempt\n");
-		return -1;
+		return -EIO;
 	}
 
 	return 0;
@@ -484,10 +484,22 @@ static int usb_pcwd_open(struct inode *inode, struct file *file)
 		goto error;
 
 	/* Activate */
-	usb_pcwd_start(usb_pcwd_device);
-	usb_pcwd_keepalive(usb_pcwd_device);
+	retval = usb_pcwd_start(usb_pcwd_device);
+	if (retval < 0)
+		goto err_bail;
+	retval = usb_pcwd_keepalive(usb_pcwd_device);
+	if (retval < 0)
+		goto err_bail_and_stop;
 	retval = stream_open(inode, file);
+	if (retval < 0)
+		goto err_bail_and_stop;
+	mutex_unlock(&disconnect_mutex);
+	return retval;
 
+err_bail_and_stop:
+	usb_pcwd_stop(usb_pcwd_device);
+err_bail:
+	clear_bit(0, &is_active);
 error:
 	mutex_unlock(&disconnect_mutex);
 	return retval;
-- 
2.40.0

[PATCH 4/8] pcwd_usb: do not leave a freed URB active

[Oliver Neukum]

Fix disconnect so that the URB is killed.
Not doing so leaves this to error handling
in the case of physical disconnect.
This fixes the case of a soft disconnect
and prevents multiple accesses to freed
memory.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/watchdog/pcwd_usb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/pcwd_usb.c b/drivers/watchdog/pcwd_usb.c
index ed3be8926a15..fe58ec84ce8c 100644
--- a/drivers/watchdog/pcwd_usb.c
+++ b/drivers/watchdog/pcwd_usb.c
@@ -815,6 +815,7 @@ static void usb_pcwd_disconnect(struct usb_interface *interface)
 
 	/* We should now stop communicating with the USB PCWD device */
 	usb_pcwd->exists = 0;
+	usb_kill_urb(usb_pcwd->intr_urb);
 
 	/* Deregister */
 	misc_deregister(&usb_pcwd_miscdev);
-- 
2.40.0

[PATCH 5/8] pcwd_usb: usb_pcwd_ioctl: return IO errors

[Oliver Neukum]

Reporting back to user space that a watchdog
is disabled although it is not due to an IO error
is evil. Pass through errors.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/watchdog/pcwd_usb.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/pcwd_usb.c b/drivers/watchdog/pcwd_usb.c
index fe58ec84ce8c..b99b8fee2873 100644
--- a/drivers/watchdog/pcwd_usb.c
+++ b/drivers/watchdog/pcwd_usb.c
@@ -420,22 +420,30 @@ static long usb_pcwd_ioctl(struct file *file, unsigned int cmd,
 
 	case WDIOC_SETOPTIONS:
 	{
-		int new_options, retval = -EINVAL;
+		int new_options, retval = 0;
+		int r;
+		bool specified_something = false;
 
 		if (get_user(new_options, p))
 			return -EFAULT;
 
 		if (new_options & WDIOS_DISABLECARD) {
-			usb_pcwd_stop(usb_pcwd_device);
-			retval = 0;
+			r = usb_pcwd_stop(usb_pcwd_device);
+			retval = r < 0 ? -EIO : 0;
+			specified_something = true;
 		}
 
+		/*
+		 * technically both options are combinable
+		 * that makes error reporting tricky
+		 */
 		if (new_options & WDIOS_ENABLECARD) {
-			usb_pcwd_start(usb_pcwd_device);
-			retval = 0;
+			r = usb_pcwd_start(usb_pcwd_device);
+			retval = retval < 0 ? retval : (r < 0 ? -EIO : 0);
+			specified_something = true;
 		}
 
-		return retval;
+		return specified_something ? retval : -EINVAL;
 	}
 
 	case WDIOC_KEEPALIVE:
-- 
2.40.0

[PATCH 6/8] pcwd_usb: memory ordering of interrupt and waiter

[Oliver Neukum]

The driver manually waits in a loop for an atomic flag.
Hence memory ordering must be constrained, so that
the waiters read what the interrupt handler writes
before it touches the flag.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/watchdog/pcwd_usb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/watchdog/pcwd_usb.c b/drivers/watchdog/pcwd_usb.c
index b99b8fee2873..50ba88019ed6 100644
--- a/drivers/watchdog/pcwd_usb.c
+++ b/drivers/watchdog/pcwd_usb.c
@@ -193,6 +193,7 @@ static void usb_pcwd_intr_done(struct urb *urb)
 	usb_pcwd->cmd_command  = data[0];
 	usb_pcwd->cmd_data_msb = data[1];
 	usb_pcwd->cmd_data_lsb = data[2];
+	smp_wmb(); /* make sure waiters read them */
 
 	/* notify anyone waiting that the cmd has finished */
 	atomic_set(&usb_pcwd->cmd_received, 1);
@@ -250,6 +251,7 @@ static int usb_pcwd_send_command(struct usb_pcwd_private *usb_pcwd,
 			got_response = 1;
 	}
 
+	smp_rmb(); /* ordering with respect to interrupt handler */
 	if ((got_response) && (cmd == usb_pcwd->cmd_command)) {
 		/* read back response */
 		*msb = usb_pcwd->cmd_data_msb;
-- 
2.40.0

[PATCH 7/8] usb_pcwd: WDIOC_SETTIMEOUT: prevent preemption

[Oliver Neukum]

Delays between altering the watchdog
and giving the keepalive need to be controlled.
Do not allow preemption.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/watchdog/pcwd_usb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/pcwd_usb.c b/drivers/watchdog/pcwd_usb.c
index 50ba88019ed6..09cae7a6ad07 100644
--- a/drivers/watchdog/pcwd_usb.c
+++ b/drivers/watchdog/pcwd_usb.c
@@ -459,10 +459,14 @@ static long usb_pcwd_ioctl(struct file *file, unsigned int cmd,
 		if (get_user(new_heartbeat, p))
 			return -EFAULT;
 
-		if (usb_pcwd_set_heartbeat(usb_pcwd_device, new_heartbeat))
+		preempt_disable(); /* we are on a clock now */
+		if (usb_pcwd_set_heartbeat(usb_pcwd_device, new_heartbeat)) {
+			preempt_enable();
 			return -EINVAL;
+		}
 
 		usb_pcwd_keepalive(usb_pcwd_device);
+		preempt_enable();
 	}
 		fallthrough;
 
-- 
2.40.0

[PATCH 8/8] usb_pcwd: remove superfluous usb_device pointer

[Oliver Neukum]

Retrieve the device from the interface

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/watchdog/pcwd_usb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/pcwd_usb.c b/drivers/watchdog/pcwd_usb.c
index 09cae7a6ad07..055acc191af9 100644
--- a/drivers/watchdog/pcwd_usb.c
+++ b/drivers/watchdog/pcwd_usb.c
@@ -112,8 +112,6 @@ static char expect_release;
 
 /* Structure to hold all of our device specific stuff */
 struct usb_pcwd_private {
-	/* save off the usb device pointer */
-	struct usb_device	*udev;
 	/* the interface for this device */
 	struct usb_interface	*interface;
 
@@ -210,6 +208,7 @@ static int usb_pcwd_send_command(struct usb_pcwd_private *usb_pcwd,
 {
 	int got_response, count;
 	unsigned char *buf;
+	struct usb_device *udev = interface_to_usbdev(usb_pcwd->interface);
 
 	/* We will not send any commands if the USB PCWD device does
 	 * not exist */
@@ -233,7 +232,7 @@ static int usb_pcwd_send_command(struct usb_pcwd_private *usb_pcwd,
 
 	atomic_set(&usb_pcwd->cmd_received, 0);
 
-	if (usb_control_msg(usb_pcwd->udev, usb_sndctrlpipe(usb_pcwd->udev, 0),
+	if (usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
 			HID_REQ_SET_REPORT, HID_DT_REPORT,
 			0x0200, usb_pcwd->interface_number, buf, 6,
 			USB_COMMAND_TIMEOUT) != 6) {
@@ -625,8 +624,10 @@ static struct notifier_block usb_pcwd_notifier = {
  */
 static inline void usb_pcwd_delete(struct usb_pcwd_private *usb_pcwd)
 {
+	struct usb_device *udev = interface_to_usbdev(usb_pcwd->interface);
+
 	usb_free_urb(usb_pcwd->intr_urb);
-	usb_free_coherent(usb_pcwd->udev, usb_pcwd->intr_size,
+	usb_free_coherent(udev, usb_pcwd->intr_size,
 			  usb_pcwd->intr_buffer, usb_pcwd->intr_dma);
 	kfree(usb_pcwd);
 }
@@ -691,7 +692,6 @@ static int usb_pcwd_probe(struct usb_interface *interface,
 	usb_pcwd_device = usb_pcwd;
 
 	mutex_init(&usb_pcwd->mtx);
-	usb_pcwd->udev = udev;
 	usb_pcwd->interface = interface;
 	usb_pcwd->interface_number = iface_desc->desc.bInterfaceNumber;
 	usb_pcwd->intr_size = (le16_to_cpu(endpoint->wMaxPacketSize) > 8 ?
-- 
2.40.0

Re: [PATCH 8/8] usb_pcwd: remove superfluous usb_device pointer

[Dan Carpenter]

Hi Oliver,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Oliver-Neukum/pcwd_usb-stop-open-race-disconnect/20230427-223413
base:   linus/master
patch link:    https://lore.kernel.org/r/20230427133350.31064-9-oneukum%40suse.com
patch subject: [PATCH 8/8] usb_pcwd: remove superfluous usb_device pointer
config: parisc-randconfig-m041-20230509 (https://download.01.org/0day-ci/archive/20230512/202305122231.n8RlGMhT-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202305122231.n8RlGMhT-lkp@intel.com/

smatch warnings:
drivers/watchdog/pcwd_usb.c:215 usb_pcwd_send_command() warn: variable dereferenced before check 'usb_pcwd' (see line 211)

vim +/usb_pcwd +215 drivers/watchdog/pcwd_usb.c

143a2e54bf5321 drivers/watchdog/pcwd_usb.c      Wim Van Sebroeck   2009-03-18  206  static int usb_pcwd_send_command(struct usb_pcwd_private *usb_pcwd,
143a2e54bf5321 drivers/watchdog/pcwd_usb.c      Wim Van Sebroeck   2009-03-18  207  		unsigned char cmd, unsigned char *msb, unsigned char *lsb)
^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16  208  {
^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16  209  	int got_response, count;
5412df0bda90a2 drivers/watchdog/pcwd_usb.c      Guenter Roeck      2013-10-14  210  	unsigned char *buf;
41b4f0869562d7 drivers/watchdog/pcwd_usb.c      Oliver Neukum      2023-04-27 @211  	struct usb_device *udev = interface_to_usbdev(usb_pcwd->interface);
                                                                                                                                      ^^^^^^^^^^
Dereference.

^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16  212  
143a2e54bf5321 drivers/watchdog/pcwd_usb.c      Wim Van Sebroeck   2009-03-18  213  	/* We will not send any commands if the USB PCWD device does
143a2e54bf5321 drivers/watchdog/pcwd_usb.c      Wim Van Sebroeck   2009-03-18  214  	 * not exist */
^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16 @215  	if ((!usb_pcwd) || (!usb_pcwd->exists))
                                                                                            ^^^^^^^^^^^
Checked too late.

^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16  216  		return -1;
^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16  217  
5412df0bda90a2 drivers/watchdog/pcwd_usb.c      Guenter Roeck      2013-10-14  218  	buf = kmalloc(6, GFP_KERNEL);
5412df0bda90a2 drivers/watchdog/pcwd_usb.c      Guenter Roeck      2013-10-14  219  	if (buf == NULL)
5412df0bda90a2 drivers/watchdog/pcwd_usb.c      Guenter Roeck      2013-10-14  220  		return 0;
5412df0bda90a2 drivers/watchdog/pcwd_usb.c      Guenter Roeck      2013-10-14  221  
143a2e54bf5321 drivers/watchdog/pcwd_usb.c      Wim Van Sebroeck   2009-03-18  222  	/* The USB PC Watchdog uses a 6 byte report format.
143a2e54bf5321 drivers/watchdog/pcwd_usb.c      Wim Van Sebroeck   2009-03-18  223  	 * The board currently uses only 3 of the six bytes of the report. */
^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16  224  	buf[0] = cmd;			/* Byte 0 = CMD */
^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16  225  	buf[1] = *msb;			/* Byte 1 = Data MSB */
^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16  226  	buf[2] = *lsb;			/* Byte 2 = Data LSB */
^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16  227  	buf[3] = buf[4] = buf[5] = 0;	/* All other bytes not used */
^1da177e4c3f41 drivers/char/watchdog/pcwd_usb.c Linus Torvalds     2005-04-16  228  
d7e92f7f768477 drivers/watchdog/pcwd_usb.c      Greg Kroah-Hartman 2013-12-19  229  	dev_dbg(&usb_pcwd->interface->dev,

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: watchdog: pcwd driver updates (was: No subject)

[Guenter Roeck]

Oliver,

On 4/27/23 06:33, Oliver Neukum wrote:
> This fixes some long standing deficiencies in error handling,
> several race conditions and disconnect handling.
> Finally a cleanup as we now can get the device easily
> from the interface.
> 

This series is a no-go. If you want to improve the driver, please
convert it to use the watchdog subsystem API.

Please note that the subject of your patches should start with
"watchdog: pcwd:"

Thanks,
Guenter

Re: watchdog: pcwd driver updates (was: No subject)

[Oliver Neukum]

On 27.04.23 16:12, Guenter Roeck wrote:
> Oliver,
> 
> On 4/27/23 06:33, Oliver Neukum wrote:
>> This fixes some long standing deficiencies in error handling,
>> several race conditions and disconnect handling.
>> Finally a cleanup as we now can get the device easily
>> from the interface.
>>
> 
> This series is a no-go. If you want to improve the driver, please
> convert it to use the watchdog subsystem API.
> 
> Please note that the subject of your patches should start with
> "watchdog: pcwd:"

Hi,

this would be problematic, because I do not have the hardware
and given its age I won't. I certainly will break the driver
if I do this extensive a change without testing it.

However, as is the driver has obvious issues, which I can fix.
We can either do the sensible fixes or let it quietly rot.

	Regards
		Oliver

Re: watchdog: pcwd driver updates (was: No subject)

[Guenter Roeck]

On 4/27/23 07:19, Oliver Neukum wrote:
> 
> 
> On 27.04.23 16:12, Guenter Roeck wrote:
>> Oliver,
>>
>> On 4/27/23 06:33, Oliver Neukum wrote:
>>> This fixes some long standing deficiencies in error handling,
>>> several race conditions and disconnect handling.
>>> Finally a cleanup as we now can get the device easily
>>> from the interface.
>>>
>>
>> This series is a no-go. If you want to improve the driver, please
>> convert it to use the watchdog subsystem API.
>>
>> Please note that the subject of your patches should start with
>> "watchdog: pcwd:"
> 
> Hi,
> 
> this would be problematic, because I do not have the hardware
> and given its age I won't. I certainly will break the driver
> if I do this extensive a change without testing it.
> 
> However, as is the driver has obvious issues, which I can fix.
> We can either do the sensible fixes or let it quietly rot.
> 

Several of those issues would be solved by using the watchdog subsystem.

I am not going to review patches for watchdog drivers not using
the watchdog subsystem. I would suggest to refrain from making changes
to such drivers, even more so if you don't have the hardware to test
those changes.

Thanks,
Guenter