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 <[email protected]>
---
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
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 <[email protected]>
---
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
Retrieve the device from the interface
Signed-off-by: Oliver Neukum <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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
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
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/[email protected]/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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Link: https://lore.kernel.org/r/[email protected]/
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