* [PATCH 1/8] pwcd_usb: fix error handling in probe
[not found] <20230427133350.31064-1-oneukum@suse.com>
@ 2023-04-27 13:33 ` Oliver Neukum
2023-04-27 13:33 ` [PATCH 2/8] pcwd_usb: stop open race disconnect Oliver Neukum
` (7 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Oliver Neukum @ 2023-04-27 13:33 UTC (permalink / raw)
To: wim, linux, linux-watchdog, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/8] pcwd_usb: stop open race disconnect
[not found] <20230427133350.31064-1-oneukum@suse.com>
2023-04-27 13:33 ` [PATCH 1/8] pwcd_usb: fix error handling in probe Oliver Neukum
@ 2023-04-27 13:33 ` Oliver Neukum
2023-04-27 13:33 ` [PATCH 3/8] pcwd_usb: usb_pcwd_open handle and return errors Oliver Neukum
` (6 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Oliver Neukum @ 2023-04-27 13:33 UTC (permalink / raw)
To: wim, linux, linux-watchdog, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/8] pcwd_usb: usb_pcwd_open handle and return errors
[not found] <20230427133350.31064-1-oneukum@suse.com>
2023-04-27 13:33 ` [PATCH 1/8] pwcd_usb: fix error handling in probe Oliver Neukum
2023-04-27 13:33 ` [PATCH 2/8] pcwd_usb: stop open race disconnect Oliver Neukum
@ 2023-04-27 13:33 ` Oliver Neukum
2023-04-27 13:33 ` [PATCH 4/8] pcwd_usb: do not leave a freed URB active Oliver Neukum
` (5 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Oliver Neukum @ 2023-04-27 13:33 UTC (permalink / raw)
To: wim, linux, linux-watchdog, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/8] pcwd_usb: do not leave a freed URB active
[not found] <20230427133350.31064-1-oneukum@suse.com>
` (2 preceding siblings ...)
2023-04-27 13:33 ` [PATCH 3/8] pcwd_usb: usb_pcwd_open handle and return errors Oliver Neukum
@ 2023-04-27 13:33 ` Oliver Neukum
2023-04-27 13:33 ` [PATCH 5/8] pcwd_usb: usb_pcwd_ioctl: return IO errors Oliver Neukum
` (4 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Oliver Neukum @ 2023-04-27 13:33 UTC (permalink / raw)
To: wim, linux, linux-watchdog, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 5/8] pcwd_usb: usb_pcwd_ioctl: return IO errors
[not found] <20230427133350.31064-1-oneukum@suse.com>
` (3 preceding siblings ...)
2023-04-27 13:33 ` [PATCH 4/8] pcwd_usb: do not leave a freed URB active Oliver Neukum
@ 2023-04-27 13:33 ` Oliver Neukum
2023-04-27 13:33 ` [PATCH 6/8] pcwd_usb: memory ordering of interrupt and waiter Oliver Neukum
` (3 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Oliver Neukum @ 2023-04-27 13:33 UTC (permalink / raw)
To: wim, linux, linux-watchdog, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 6/8] pcwd_usb: memory ordering of interrupt and waiter
[not found] <20230427133350.31064-1-oneukum@suse.com>
` (4 preceding siblings ...)
2023-04-27 13:33 ` [PATCH 5/8] pcwd_usb: usb_pcwd_ioctl: return IO errors Oliver Neukum
@ 2023-04-27 13:33 ` Oliver Neukum
2023-04-27 13:33 ` [PATCH 7/8] usb_pcwd: WDIOC_SETTIMEOUT: prevent preemption Oliver Neukum
` (2 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Oliver Neukum @ 2023-04-27 13:33 UTC (permalink / raw)
To: wim, linux, linux-watchdog, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 7/8] usb_pcwd: WDIOC_SETTIMEOUT: prevent preemption
[not found] <20230427133350.31064-1-oneukum@suse.com>
` (5 preceding siblings ...)
2023-04-27 13:33 ` [PATCH 6/8] pcwd_usb: memory ordering of interrupt and waiter Oliver Neukum
@ 2023-04-27 13:33 ` Oliver Neukum
2023-04-27 13:33 ` [PATCH 8/8] usb_pcwd: remove superfluous usb_device pointer Oliver Neukum
2023-04-27 14:12 ` watchdog: pcwd driver updates (was: No subject) Guenter Roeck
8 siblings, 0 replies; 12+ messages in thread
From: Oliver Neukum @ 2023-04-27 13:33 UTC (permalink / raw)
To: wim, linux, linux-watchdog, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 8/8] usb_pcwd: remove superfluous usb_device pointer
[not found] <20230427133350.31064-1-oneukum@suse.com>
` (6 preceding siblings ...)
2023-04-27 13:33 ` [PATCH 7/8] usb_pcwd: WDIOC_SETTIMEOUT: prevent preemption Oliver Neukum
@ 2023-04-27 13:33 ` Oliver Neukum
2023-05-13 8:52 ` Dan Carpenter
2023-04-27 14:12 ` watchdog: pcwd driver updates (was: No subject) Guenter Roeck
8 siblings, 1 reply; 12+ messages in thread
From: Oliver Neukum @ 2023-04-27 13:33 UTC (permalink / raw)
To: wim, linux, linux-watchdog, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: watchdog: pcwd driver updates (was: No subject)
[not found] <20230427133350.31064-1-oneukum@suse.com>
` (7 preceding siblings ...)
2023-04-27 13:33 ` [PATCH 8/8] usb_pcwd: remove superfluous usb_device pointer Oliver Neukum
@ 2023-04-27 14:12 ` Guenter Roeck
2023-04-27 14:19 ` Oliver Neukum
8 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2023-04-27 14:12 UTC (permalink / raw)
To: Oliver Neukum, wim, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: watchdog: pcwd driver updates (was: No subject)
2023-04-27 14:12 ` watchdog: pcwd driver updates (was: No subject) Guenter Roeck
@ 2023-04-27 14:19 ` Oliver Neukum
2023-04-27 14:35 ` Guenter Roeck
0 siblings, 1 reply; 12+ messages in thread
From: Oliver Neukum @ 2023-04-27 14:19 UTC (permalink / raw)
To: Guenter Roeck, Oliver Neukum, wim, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: watchdog: pcwd driver updates (was: No subject)
2023-04-27 14:19 ` Oliver Neukum
@ 2023-04-27 14:35 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2023-04-27 14:35 UTC (permalink / raw)
To: Oliver Neukum, wim, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 8/8] usb_pcwd: remove superfluous usb_device pointer
2023-04-27 13:33 ` [PATCH 8/8] usb_pcwd: remove superfluous usb_device pointer Oliver Neukum
@ 2023-05-13 8:52 ` Dan Carpenter
0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2023-05-13 8:52 UTC (permalink / raw)
To: oe-kbuild, Oliver Neukum, wim, linux, linux-watchdog,
linux-kernel
Cc: lkp, oe-kbuild-all, Oliver Neukum
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-05-13 8:52 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230427133350.31064-1-oneukum@suse.com>
2023-04-27 13:33 ` [PATCH 1/8] pwcd_usb: fix error handling in probe Oliver Neukum
2023-04-27 13:33 ` [PATCH 2/8] pcwd_usb: stop open race disconnect Oliver Neukum
2023-04-27 13:33 ` [PATCH 3/8] pcwd_usb: usb_pcwd_open handle and return errors Oliver Neukum
2023-04-27 13:33 ` [PATCH 4/8] pcwd_usb: do not leave a freed URB active Oliver Neukum
2023-04-27 13:33 ` [PATCH 5/8] pcwd_usb: usb_pcwd_ioctl: return IO errors Oliver Neukum
2023-04-27 13:33 ` [PATCH 6/8] pcwd_usb: memory ordering of interrupt and waiter Oliver Neukum
2023-04-27 13:33 ` [PATCH 7/8] usb_pcwd: WDIOC_SETTIMEOUT: prevent preemption Oliver Neukum
2023-04-27 13:33 ` [PATCH 8/8] usb_pcwd: remove superfluous usb_device pointer Oliver Neukum
2023-05-13 8:52 ` Dan Carpenter
2023-04-27 14:12 ` watchdog: pcwd driver updates (was: No subject) Guenter Roeck
2023-04-27 14:19 ` Oliver Neukum
2023-04-27 14:35 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox