Re: usbtmc: vendor specific i/o
On Wed, Sep 28, 2016 at 05:25:42PM +0200, Dave Penkler wrote: > On Wed, Sep 28, 2016 at 12:29:35PM +0200, Ladislav Michl wrote: > > On Wed, Sep 28, 2016 at 11:45:02AM +0200, Greg Kroah-Hartman wrote: > > > If it is freed, why is a read even able to happen? Ah, ick, no proper > > > reference counting is happening here :( > > > > > > Oh, no, wait, it is happening properly, it's just that it's not the > > > lifespan that the devm_kzalloc() is attached to, so yes, the correct fix > > > here is to revert that patch as it is incorrect. > > > > Well, reference counting is also suspicious as kref_get(&data->kref) in > > probe function with comment "will reference data in int urb" gives no > > clue why there's explicit reference. Also what if we add classic > > error unwinding and leave usb_submit_urb to open time? But wait, this > > driver allows multiple opens? Is it intentional? It could be done this > > way (note patch is only for reference as there's nothing to prevent > > multiple open and therefore multiple usb_submit_urb): > > > > The usbtmc_interrupt routine will reference data in the int urb from > the interrupt context. Also submitting the urb on probe makes for > simpler housekeeping. Only one interrupt urb needs to be submitted no > matter how many concurrent opens there are for the minor. I will > submit a patch to revert the "convert to devm_kzalloc" patch and make > the comment more explicit. I've already merged such a commit, but if you want to document it better, that would be wonderful. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbtmc: vendor specific i/o
On Wed, Sep 28, 2016 at 12:29:35PM +0200, Ladislav Michl wrote: > On Wed, Sep 28, 2016 at 11:45:02AM +0200, Greg Kroah-Hartman wrote: > > If it is freed, why is a read even able to happen? Ah, ick, no proper > > reference counting is happening here :( > > > > Oh, no, wait, it is happening properly, it's just that it's not the > > lifespan that the devm_kzalloc() is attached to, so yes, the correct fix > > here is to revert that patch as it is incorrect. > > Well, reference counting is also suspicious as kref_get(&data->kref) in > probe function with comment "will reference data in int urb" gives no > clue why there's explicit reference. Also what if we add classic > error unwinding and leave usb_submit_urb to open time? But wait, this > driver allows multiple opens? Is it intentional? It could be done this > way (note patch is only for reference as there's nothing to prevent > multiple open and therefore multiple usb_submit_urb): > The usbtmc_interrupt routine will reference data in the int urb from the interrupt context. Also submitting the urb on probe makes for simpler housekeeping. Only one interrupt urb needs to be submitted no matter how many concurrent opens there are for the minor. I will submit a patch to revert the "convert to devm_kzalloc" patch and make the comment more explicit. cheers, -dave -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbtmc: vendor specific i/o
On Wed, Sep 28, 2016 at 12:29:35PM +0200, Ladislav Michl wrote: > On Wed, Sep 28, 2016 at 11:45:02AM +0200, Greg Kroah-Hartman wrote: > > If it is freed, why is a read even able to happen? Ah, ick, no proper > > reference counting is happening here :( > > > > Oh, no, wait, it is happening properly, it's just that it's not the > > lifespan that the devm_kzalloc() is attached to, so yes, the correct fix > > here is to revert that patch as it is incorrect. > > Well, reference counting is also suspicious as kref_get(&data->kref) in > probe function with comment "will reference data in int urb" gives no > clue why there's explicit reference. Also what if we add classic > error unwinding and leave usb_submit_urb to open time? But wait, this > driver allows multiple opens? Is it intentional? It could be done this > way (note patch is only for reference as there's nothing to prevent > multiple open and therefore multiple usb_submit_urb): Oh, I don't doubt there are problems here, luckily very few people actually use the driver in ways that could stress it :) I'll gladly take any fixup patches you might have for it, especially if you can test them as I don't have this type of hardware. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbtmc: vendor specific i/o
On Wed, 2016-09-28 at 12:29 +0200, Ladislav Michl wrote: > Well, reference counting is also suspicious as kref_get(&data->kref) > in > probe function with comment "will reference data in int urb" gives no > clue why there's explicit reference. Also what if we add classic > error unwinding and leave usb_submit_urb to open time? But wait, this > driver allows multiple opens? Is it intentional? It could be done this > way (note patch is only for reference as there's nothing to prevent > multiple open and therefore multiple usb_submit_urb): You need to check "zombie" in open() or you open up to another race. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbtmc: vendor specific i/o
On Wed, Sep 28, 2016 at 11:45:02AM +0200, Greg Kroah-Hartman wrote:
> If it is freed, why is a read even able to happen? Ah, ick, no proper
> reference counting is happening here :(
>
> Oh, no, wait, it is happening properly, it's just that it's not the
> lifespan that the devm_kzalloc() is attached to, so yes, the correct fix
> here is to revert that patch as it is incorrect.
Well, reference counting is also suspicious as kref_get(&data->kref) in
probe function with comment "will reference data in int urb" gives no
clue why there's explicit reference. Also what if we add classic
error unwinding and leave usb_submit_urb to open time? But wait, this
driver allows multiple opens? Is it intentional? It could be done this
way (note patch is only for reference as there's nothing to prevent
multiple open and therefore multiple usb_submit_urb):
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 917a55c..0964e49 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -147,7 +147,6 @@ static int usbtmc_open(struct inode *inode, struct file
*filp)
{
struct usb_interface *intf;
struct usbtmc_device_data *data;
- int retval = 0;
intf = usb_find_interface(&usbtmc_driver, iminor(inode));
if (!intf) {
@@ -156,12 +155,19 @@ static int usbtmc_open(struct inode *inode, struct file
*filp)
}
data = usb_get_intfdata(intf);
- kref_get(&data->kref);
+ if (data->iin_ep_present) {
+ int retval = usb_submit_urb(data->iin_urb, GFP_KERNEL);
+ if (retval) {
+ dev_err(&intf->dev, "Failed to submit iin_urb\n");
+ return retval;
+ }
+ }
/* Store pointer in file structure's private data field */
filp->private_data = data;
+ kref_get(&data->kref);
- return retval;
+ return 0;
}
static int usbtmc_release(struct inode *inode, struct file *file)
@@ -1358,16 +1364,6 @@ exit:
dev_err(dev, "usb_submit_urb failed: %d\n", rv);
}
-static void usbtmc_free_int(struct usbtmc_device_data *data)
-{
- if (!data->iin_ep_present || !data->iin_urb)
- return;
- usb_kill_urb(data->iin_urb);
- kfree(data->iin_buffer);
- usb_free_urb(data->iin_urb);
- kref_put(&data->kref, usbtmc_delete);
-}
-
static int usbtmc_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -1469,18 +1465,16 @@ static int usbtmc_probe(struct usb_interface *intf,
data->iin_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!data->iin_urb) {
dev_err(&intf->dev, "Failed to allocate int urb\n");
- goto error_register;
+ goto error_alloc_urb;
}
- /* will reference data in int urb */
- kref_get(&data->kref);
/* allocate buffer for interrupt in */
data->iin_buffer = kmalloc(data->iin_wMaxPacketSize,
GFP_KERNEL);
if (!data->iin_buffer) {
dev_err(&intf->dev, "Failed to allocate int buf\n");
- goto error_register;
+ goto error_alloc_buffer;
}
/* fill interrupt urb */
@@ -1490,11 +1484,6 @@ static int usbtmc_probe(struct usb_interface *intf,
usbtmc_interrupt,
data, data->iin_interval);
- retcode = usb_submit_urb(data->iin_urb, GFP_KERNEL);
- if (retcode) {
- dev_err(&intf->dev, "Failed to submit iin_urb\n");
- goto error_register;
- }
}
retcode = sysfs_create_group(&intf->dev.kobj, &data_attr_grp);
@@ -1511,10 +1500,16 @@ static int usbtmc_probe(struct usb_interface *intf,
return 0;
error_register:
- sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
- usbtmc_free_int(data);
- kref_put(&data->kref, usbtmc_delete);
+ if (data->iin_ep_present) {
+ kfree(data->iin_buffer);
+error_alloc_buffer:
+ usb_free_urb(data->iin_urb);
+ }
+error_alloc_urb:
+ sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
+ usb_put_dev(data->usb_dev);
+
return retcode;
}
@@ -1532,7 +1527,11 @@ static void usbtmc_disconnect(struct usb_interface *intf)
data->zombie = 1;
wake_up_all(&data->waitq);
mutex_unlock(&data->io_mutex);
- usbtmc_free_int(data);
+ if (data->iin_ep_present) {
+ usb_kill_urb(data->iin_urb);
+ kfree(data->iin_buffer);
+ usb_free_urb(data->iin_urb);
+ }
kref_put(&data->kref, usbtmc_delete);
}
--
To unsubscri
Re: usbtmc: vendor specific i/o
On Wed, Sep 28, 2016 at 08:38:07AM +0200, Ladislav Michl wrote: > On Thu, Aug 04, 2016 at 10:15:33PM +0200, Ladislav Michl wrote: > > On Thu, Aug 04, 2016 at 06:59:30AM +0200, Greg Kroah-Hartman wrote: > > > On Wed, Aug 03, 2016 at 09:06:25AM +0200, Ladislav Michl wrote: > > > > Yes, also was lacking proper description and signoff. So, I'm > > > > considering > > > > ioctls based approach okay, although that question (the only one I > > > > really > > > > had) was never answered. > > > > > > > > After re-reading specifications [*] I decided to allow arbitrary MsgID > > > > selection, as USB488 adds MsgID=TRIGGER (128) and other subclass > > > > specifications may add other values. > > > > > > > > [*] http://www.usb.org/developers/docs/devclass_docs/USBTMC_1_006a.zip > > > > > > > > After sorting out all eventual objections, patch bellow will be turned > > > > into proper one. > > > > > > Looks reasonable to me. > > > > After all, it is not that reasonable. Spec does not define anything like > > EOM or TermCharEnabled bits in bmTransferAttributes nor TermChar field > > in vendor specific messages - usbtmc_read and usbtmc_write are using > > these fields when concatenating usb transfers. I'll need to think a bit > > more about it... > > Hmm, sorry for a delay, I was working on something else for a while. > So far using libusb seems to be better approach, but I do not want to > leave bugs I know about unfixed, so here we go... > > > Meanwhile, there's a small race condition, which needs to be fixed first. > > When device is disconnected there's a chance usbtmc_read tries to lock > > already destroyd mutex, see bellow. Fix will come later in separate patch. > > Problem comes with commit e6c7efdcb76f11b04e3d3f71c8d764ab75c9423b (usbtmc: > convert to devm_kzalloc). As memory allocated with devm_kzalloc is > automatically freed on driver detach, later call to usbtmc_read will crash > as bellow. Anyone has better fix than reverting that patch? If it is freed, why is a read even able to happen? Ah, ick, no proper reference counting is happening here :( Oh, no, wait, it is happening properly, it's just that it's not the lifespan that the devm_kzalloc() is attached to, so yes, the correct fix here is to revert that patch as it is incorrect. Nice catch, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbtmc: vendor specific i/o
On Thu, Aug 04, 2016 at 10:15:33PM +0200, Ladislav Michl wrote: > On Thu, Aug 04, 2016 at 06:59:30AM +0200, Greg Kroah-Hartman wrote: > > On Wed, Aug 03, 2016 at 09:06:25AM +0200, Ladislav Michl wrote: > > > Yes, also was lacking proper description and signoff. So, I'm considering > > > ioctls based approach okay, although that question (the only one I really > > > had) was never answered. > > > > > > After re-reading specifications [*] I decided to allow arbitrary MsgID > > > selection, as USB488 adds MsgID=TRIGGER (128) and other subclass > > > specifications may add other values. > > > > > > [*] http://www.usb.org/developers/docs/devclass_docs/USBTMC_1_006a.zip > > > > > > After sorting out all eventual objections, patch bellow will be turned > > > into proper one. > > > > Looks reasonable to me. > > After all, it is not that reasonable. Spec does not define anything like > EOM or TermCharEnabled bits in bmTransferAttributes nor TermChar field > in vendor specific messages - usbtmc_read and usbtmc_write are using > these fields when concatenating usb transfers. I'll need to think a bit > more about it... Hmm, sorry for a delay, I was working on something else for a while. So far using libusb seems to be better approach, but I do not want to leave bugs I know about unfixed, so here we go... > Meanwhile, there's a small race condition, which needs to be fixed first. > When device is disconnected there's a chance usbtmc_read tries to lock > already destroyd mutex, see bellow. Fix will come later in separate patch. Problem comes with commit e6c7efdcb76f11b04e3d3f71c8d764ab75c9423b (usbtmc: convert to devm_kzalloc). As memory allocated with devm_kzalloc is automatically freed on driver detach, later call to usbtmc_read will crash as bellow. Anyone has better fix than reverting that patch? > Unable to handle kernel NULL pointer dereference at virtual address 0008 > pgd = cd55c000 > [0008] *pgd=8d52e831, *pte=, *ppte= > Internal error: Oops: 17 [#1] ARM > Modules linked in: usbtmc ppp_deflate bsd_comp ppp_async crc_ccitt > ppp_generic slhc cpufreq_dt udlfb syscopyarea sysfillrect sysimgblt > fb_sys_fops omap_aes omap_sham crypto_engine omap_mailbox option cdc_acm > usb_wwan usbserial usb_storage scsi_mod > CPU: 0 PID: 205 Comm: tvm3 Tainted: GW 4.6.0 #1 > Hardware name: Generic OMAP36xx (Flattened Device Tree) > task: ce2ae700 ti: ce1e2000 task.ti: ce1e2000 > PC is at __bfs+0x11c/0x23c > LR is at warn_slowpath_null+0x1c/0x24 > pc : []lr : []psr: 60010093 > sp : ce1e3d30 ip : fp : c0a76388 > r10: r9 : ce1e3d74 r8 : c0a72398 > r7 : ce1e3d70 r6 : c0a76388 r5 : c0c433f4 r4 : c0a72398 > r3 : 0200 r2 : e55130aa r1 : c0951cd0 r0 : > Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none > Control: 10c5387d Table: 8d55c019 DAC: 0051 > Process tvm3 (pid: 205, stack limit = 0xce1e2210) > Stack: (0xce1e3d30 to 0xce1e4000) > 3d20: ce1e4000 c014f928 e55130aa > 3d40: 0003 0001 ce2aeab0 ce2ae700 c073ceee c0151090 > 3d60: c015113c c0c99af8 ce2aeab0 c010adc4 c0c4333c c0c432fc > 3d80: ce1e3fa8 c01074a0 0072 0001 > 3da0: 0002 ce2aeab0 c0603f60 ce2ae700 c0151cc4 ce2ae700 > 3dc0: 3c48dade 001e 0001 ce103300 ce2ae700 c0152c88 ce2a0030 > 3de0: ce2aeab0 0100 c0151fcc ce2ae700 0001 c01c6a8c 001a9ca0 > 3e00: ce80 c0152174 cd4e5980 a0010013 cfc7cf7c c01c6a8c ce245420 > 3e20: 0003 60010013 0001 c10ed4ec ce2ae700 ce1e2000 > 3e40: 0100 c015470c 0001 bf14d88c > 3e60: cd5a5a60 bf14d840 ce20c800 c05a8c94 0001 bf14d88c a0010013 > 3e80: c0107644 c0152174 ce20c800 ce800300 cd5a5a10 bf14d840 ce20c800 c10f5cc8 > 3ea0: c0107644 ce1e2000 bf14d88c ce2aeab0 0001 0051 > 3ec0: b2afeb60 cd5a5a60 ce1e3f88 0001 000670f1 c0f3c608 cd5a7a00 > 3ee0: bf14d840 ce1e3f88 b2afeb60 c0107644 ce1e2000 c01cc0b8 > 3f00: c02e34f0 0c00 cd5a7a00 0100 > 3f20: c01ccb40 4000 c0943184 0038 b2afeb60 > 3f40: cd5a7a00 ce1e3f88 b2afeb60 b2afeb60 cd5a7a00 ce1e3f88 b2afeb60 c01ccc58 > 3f60: cd5a7a00 b2afeb60 0100 cd5a7a00 cd5a7a01 0100 b2afeb60 c0107644 > 3f80: ce1e2000 c01cd834 0c00 0100 000e 000a8050 > 3fa0: 0003 c01074a0 000e 000e b2afeb60 0100 > 3fc0: 000e 000a8050 0003 000a8054 000a67cc 0008e3b4 > 3fe0: b2afeb40 b6d48b60 80010030 000e 8fef6861 8fef6c61 > [] (__bfs) from [] (check_usage_backwards+0xac/0x140) > [] (check_usage_backwards) from [] (mark_lock+0x36c/0x618) > [] (mark_lock) from [] (__lock_acquire+0x880/0x1b88) > [] (__loc
Re: usbtmc: vendor specific i/o
On Thu, Aug 04, 2016 at 06:59:30AM +0200, Greg Kroah-Hartman wrote: > On Wed, Aug 03, 2016 at 09:06:25AM +0200, Ladislav Michl wrote: > > Yes, also was lacking proper description and signoff. So, I'm considering > > ioctls based approach okay, although that question (the only one I really > > had) was never answered. > > > > After re-reading specifications [*] I decided to allow arbitrary MsgID > > selection, as USB488 adds MsgID=TRIGGER (128) and other subclass > > specifications may add other values. > > > > [*] http://www.usb.org/developers/docs/devclass_docs/USBTMC_1_006a.zip > > > > After sorting out all eventual objections, patch bellow will be turned > > into proper one. > > Looks reasonable to me. After all, it is not that reasonable. Spec does not define anything like EOM or TermCharEnabled bits in bmTransferAttributes nor TermChar field in vendor specific messages - usbtmc_read and usbtmc_write are using these fields when concatenating usb transfers. I'll need to think a bit more about it... Meanwhile, there's a small race condition, which needs to be fixed first. When device is disconnected there's a chance usbtmc_read tries to lock already destroyd mutex, see bellow. Fix will come later in separate patch. thank you, ladis Unable to handle kernel NULL pointer dereference at virtual address 0008 pgd = cd55c000 [0008] *pgd=8d52e831, *pte=, *ppte= Internal error: Oops: 17 [#1] ARM Modules linked in: usbtmc ppp_deflate bsd_comp ppp_async crc_ccitt ppp_generic slhc cpufreq_dt udlfb syscopyarea sysfillrect sysimgblt fb_sys_fops omap_aes omap_sham crypto_engine omap_mailbox option cdc_acm usb_wwan usbserial usb_storage scsi_mod CPU: 0 PID: 205 Comm: tvm3 Tainted: GW 4.6.0 #1 Hardware name: Generic OMAP36xx (Flattened Device Tree) task: ce2ae700 ti: ce1e2000 task.ti: ce1e2000 PC is at __bfs+0x11c/0x23c LR is at warn_slowpath_null+0x1c/0x24 pc : []lr : []psr: 60010093 sp : ce1e3d30 ip : fp : c0a76388 r10: r9 : ce1e3d74 r8 : c0a72398 r7 : ce1e3d70 r6 : c0a76388 r5 : c0c433f4 r4 : c0a72398 r3 : 0200 r2 : e55130aa r1 : c0951cd0 r0 : Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 8d55c019 DAC: 0051 Process tvm3 (pid: 205, stack limit = 0xce1e2210) Stack: (0xce1e3d30 to 0xce1e4000) 3d20: ce1e4000 c014f928 e55130aa 3d40: 0003 0001 ce2aeab0 ce2ae700 c073ceee c0151090 3d60: c015113c c0c99af8 ce2aeab0 c010adc4 c0c4333c c0c432fc 3d80: ce1e3fa8 c01074a0 0072 0001 3da0: 0002 ce2aeab0 c0603f60 ce2ae700 c0151cc4 ce2ae700 3dc0: 3c48dade 001e 0001 ce103300 ce2ae700 c0152c88 ce2a0030 3de0: ce2aeab0 0100 c0151fcc ce2ae700 0001 c01c6a8c 001a9ca0 3e00: ce80 c0152174 cd4e5980 a0010013 cfc7cf7c c01c6a8c ce245420 3e20: 0003 60010013 0001 c10ed4ec ce2ae700 ce1e2000 3e40: 0100 c015470c 0001 bf14d88c 3e60: cd5a5a60 bf14d840 ce20c800 c05a8c94 0001 bf14d88c a0010013 3e80: c0107644 c0152174 ce20c800 ce800300 cd5a5a10 bf14d840 ce20c800 c10f5cc8 3ea0: c0107644 ce1e2000 bf14d88c ce2aeab0 0001 0051 3ec0: b2afeb60 cd5a5a60 ce1e3f88 0001 000670f1 c0f3c608 cd5a7a00 3ee0: bf14d840 ce1e3f88 b2afeb60 c0107644 ce1e2000 c01cc0b8 3f00: c02e34f0 0c00 cd5a7a00 0100 3f20: c01ccb40 4000 c0943184 0038 b2afeb60 3f40: cd5a7a00 ce1e3f88 b2afeb60 b2afeb60 cd5a7a00 ce1e3f88 b2afeb60 c01ccc58 3f60: cd5a7a00 b2afeb60 0100 cd5a7a00 cd5a7a01 0100 b2afeb60 c0107644 3f80: ce1e2000 c01cd834 0c00 0100 000e 000a8050 3fa0: 0003 c01074a0 000e 000e b2afeb60 0100 3fc0: 000e 000a8050 0003 000a8054 000a67cc 0008e3b4 3fe0: b2afeb40 b6d48b60 80010030 000e 8fef6861 8fef6c61 [] (__bfs) from [] (check_usage_backwards+0xac/0x140) [] (check_usage_backwards) from [] (mark_lock+0x36c/0x618) [] (mark_lock) from [] (__lock_acquire+0x880/0x1b88) [] (__lock_acquire) from [] (lock_acquire+0x70/0x90) [] (lock_acquire) from [] (mutex_lock_nested+0x3c/0x314) [] (mutex_lock_nested) from [] (usbtmc_read+0x4c/0x4e8 [usbtmc]) [] (usbtmc_read [usbtmc]) from [] (__vfs_read+0x20/0xcc) [] (__vfs_read) from [] (vfs_read+0x84/0xec) [] (vfs_read) from [] (SyS_read+0x40/0x80) [] (SyS_read) from [] (ret_fast_syscall+0x0/0x1c) Code: e30013af e58d200c ebff61be e59d200c (e59a1008) ---[ end trace dd5c876458afcc20 ]--- Kernel panic - not syncing: Fatal exception -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vg
Re: usbtmc: vendor specific i/o
On Wed, Aug 03, 2016 at 09:06:25AM +0200, Ladislav Michl wrote: > On Wed, Aug 03, 2016 at 05:59:59AM +0200, Greg Kroah-Hartman wrote: > > But your patch was "one-way", once you switched to the other mode, the > > old one could not be used :( > > Yes, also was lacking proper description and signoff. So, I'm considering > ioctls based approach okay, although that question (the only one I really > had) was never answered. > > After re-reading specifications [*] I decided to allow arbitrary MsgID > selection, as USB488 adds MsgID=TRIGGER (128) and other subclass > specifications may add other values. > > [*] http://www.usb.org/developers/docs/devclass_docs/USBTMC_1_006a.zip > > After sorting out all eventual objections, patch bellow will be turned > into proper one. Looks reasonable to me. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbtmc: vendor specific i/o
On Wed, Aug 03, 2016 at 05:59:59AM +0200, Greg Kroah-Hartman wrote:
> But your patch was "one-way", once you switched to the other mode, the
> old one could not be used :(
Yes, also was lacking proper description and signoff. So, I'm considering
ioctls based approach okay, although that question (the only one I really
had) was never answered.
After re-reading specifications [*] I decided to allow arbitrary MsgID
selection, as USB488 adds MsgID=TRIGGER (128) and other subclass
specifications may add other values.
[*] http://www.usb.org/developers/docs/devclass_docs/USBTMC_1_006a.zip
After sorting out all eventual objections, patch bellow will be turned
into proper one.
Thank you,
ladis
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 917a55c..c6d3ca3 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -84,6 +84,9 @@ struct usbtmc_device_data {
unsigned int bulk_in;
unsigned int bulk_out;
+ u8 msgid_in;
+ u8 msgid_out;
+
u8 bTag;
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read; /* needed for abort */
@@ -393,6 +396,20 @@ exit:
return rv;
}
+static int usbtmc_ioctl_set_msgids(struct usbtmc_device_data *data,
+ void __user *arg)
+{
+ struct usbtmc_msg_ids val;
+
+ if (copy_from_user(&val, arg, sizeof(val)))
+ return -EFAULT;
+
+ data->msgid_in = val.in;
+ data->msgid_out = val.out;
+
+ return 0;
+}
+
static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
void __user *arg)
{
@@ -549,7 +566,7 @@ static int send_request_dev_dep_msg_in(struct
usbtmc_device_data *data, size_t t
/* Setup IO buffer for REQUEST_DEV_DEP_MSG_IN message
* Refer to class specs for details
*/
- buffer[0] = 2;
+ buffer[0] = data->msgid_in;
buffer[1] = data->bTag;
buffer[2] = ~data->bTag;
buffer[3] = 0; /* Reserved */
@@ -677,8 +694,8 @@ static ssize_t usbtmc_read(struct file *filp, char __user
*buf,
goto exit;
}
- if (buffer[0] != 2) {
- dev_err(dev, "Device sent reply with wrong
MsgID: %u != 2\n", buffer[0]);
+ if (buffer[0] != data->msgid_in) {
+ dev_err(dev, "Device sent reply with wrong
MsgID: %u != %u\n", buffer[0], data->msgid_in);
if (data->auto_abort)
usbtmc_ioctl_abort_bulk_in(data);
goto exit;
@@ -807,7 +824,7 @@ static ssize_t usbtmc_write(struct file *filp, const char
__user *buf,
}
/* Setup IO buffer for DEV_DEP_MSG_OUT message */
- buffer[0] = 1;
+ buffer[0] = data->msgid_out;
buffer[1] = data->bTag;
buffer[2] = ~data->bTag;
buffer[3] = 0; /* Reserved */
@@ -1227,6 +1244,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+ case USBTMC_IOCTL_SET_MSGIDS:
+ retval = usbtmc_ioctl_set_msgids(data, (void __user *)arg);
+ break;
+
case USBTMC488_IOCTL_GET_CAPS:
retval = copy_to_user((void __user *)arg,
&data->usb488_caps,
@@ -1409,6 +1430,8 @@ static int usbtmc_probe(struct usb_interface *intf,
}
/* Initialize USBTMC bTag and other fields */
+ data->msgid_in = 2;
+ data->msgid_out = 1;
data->bTag = 1;
data->TermCharEnabled = 0;
data->TermChar = '\n';
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 2e59d9c..46f21cc 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -37,6 +37,12 @@
#define USBTMC488_REQUEST_GOTO_LOCAL 161
#define USBTMC488_REQUEST_LOCAL_LOCKOUT162
+/* USB TMC command messages ids */
+struct usbtmc_msg_ids {
+ __u8 in;
+ __u8 out;
+};
+
/* Request values for USBTMC driver's ioctl entry point */
#define USBTMC_IOC_NR 91
#define USBTMC_IOCTL_INDICATOR_PULSE _IO(USBTMC_IOC_NR, 1)
@@ -45,6 +51,7 @@
#define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4)
#define USBTMC_IOCTL_CLEAR_OUT_HALT_IO(USBTMC_IOC_NR, 6)
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
+#define USBTMC_IOCTL_SET_MSGIDS_IOW(USBTMC_IOC_NR, 8, struct
usbtmc_msg_ids)
#define USBTMC488_IOCTL_GET_CAPS _IOR(USBTMC_IOC_NR, 17, unsigned char)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
#define USBTMC488_IOCTL_REN_CONTROL_IOW(USBTMC_IOC_NR, 19, unsigned char)
--
To unsubs
Re: usbtmc: vendor specific i/o
On Tue, Aug 02, 2016 at 12:09:12PM +0200, Ladislav Michl wrote: > On Tue, Aug 02, 2016 at 11:25:22AM +0200, dave penkler wrote: > >For supportability I would recommend to set the commands depending on the > >recognised HW, not with aA ioctl. See rigol quirk. What are the device > >and manufacturer ids to which these vendor extensions apply? > >/dave > > This is a custom device... If I understand USBTMC spec corectly, it is not > either "normal" message or vendor specific message, but both can happily > coexist, so I wouldn't let it depend on quirks. Once you are going to send > vendor specific message, you need to know how such a message looks like > anyway, so any quirk based magic is probably not going to help. > Also it seems both NI and IVI libs are using similar (selecting MsgID) > approach. But your patch was "one-way", once you switched to the other mode, the old one could not be used :( -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbtmc: vendor specific i/o
On Tue, Aug 02, 2016 at 11:25:22AM +0200, dave penkler wrote: >For supportability I would recommend to set the commands depending on the >recognised HW, not with aA ioctl. See rigol quirk. What are the device >and manufacturer ids to which these vendor extensions apply? >/dave This is a custom device... If I understand USBTMC spec corectly, it is not either "normal" message or vendor specific message, but both can happily coexist, so I wouldn't let it depend on quirks. Once you are going to send vendor specific message, you need to know how such a message looks like anyway, so any quirk based magic is probably not going to help. Also it seems both NI and IVI libs are using similar (selecting MsgID) approach. ladis -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbtmc: vendor specific i/o
On Tue, Aug 02, 2016 at 06:58:11AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 01, 2016 at 08:40:02PM +0200, Ladislav Michl wrote:
> > Hi,
> >
> > I need to ask usbtmc device for vendor specific data. What about adding
> > ioctl
> > which switches MsgIDs to be vendor specific? Or are there other, better,
> > options? Patch bellow ilustrates changes to allow reading and writing vendor
> > specific messages.
>
> I don't understand exactly what you are changing here, it seems like you
> are just changing the messages being sent to the device? Doesn't that
> break existing users?
Sorry for being too brief. For sure I not want to break any existing user.
Normally MsgID=DEV_DEP_MSG_OUT is used for command message,
REQUEST_DEV_DEP_MSG_IN to request response message and DEV_DEP_MSG_IN
is MsgID of response message (values are 1 and 2 respectively).
However usbtmc class defines also vendor specific messages, which uses
the same scheme with values 126, resp. 127. Currently there's no way
to send vendor specific messages.
My question is whenever it is okay to add ioctl which will alter between
normal and vendor specific messages or if there's another prefered way,
something like this pseudo-patch (showing only one way switch):
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 917a55c..4c90e50 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -549,7 +549,7 @@ static int send_request_dev_dep_msg_in(struct
usbtmc_device_data *data, size_t t
/* Setup IO buffer for REQUEST_DEV_DEP_MSG_IN message
* Refer to class specs for details
*/
- buffer[0] = 2;
+ buffer[0] = data->msgid_in;
buffer[1] = data->bTag;
buffer[2] = ~data->bTag;
buffer[3] = 0; /* Reserved */
@@ -677,8 +677,8 @@ static ssize_t usbtmc_read(struct file *filp, char __user
*buf,
goto exit;
}
- if (buffer[0] != 2) {
- dev_err(dev, "Device sent reply with wrong
MsgID: %u != 2\n", buffer[0]);
+ if (buffer[0] != data->msgid_in) {
+ dev_err(dev, "Device sent reply with wrong
MsgID: %u != %u\n", buffer[0], data->msgid_out);
if (data->auto_abort)
usbtmc_ioctl_abort_bulk_in(data);
goto exit;
@@ -807,7 +807,7 @@ static ssize_t usbtmc_write(struct file *filp, const char
__user *buf,
}
/* Setup IO buffer for DEV_DEP_MSG_OUT message */
- buffer[0] = 1;
+ buffer[0] = data->msgid_out;
buffer[1] = data->bTag;
buffer[2] = ~data->bTag;
buffer[3] = 0; /* Reserved */
@@ -1227,6 +1227,12 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+ case USBTMC_IOCTL_VENDOR_SPECIFIC:
+ data->msgid_out = 126;
+ data->msgid_in = 127;
+ retval = 0;
+ break;
+
case USBTMC488_IOCTL_GET_CAPS:
retval = copy_to_user((void __user *)arg,
&data->usb488_caps,
@@ -1414,6 +1420,8 @@ static int usbtmc_probe(struct usb_interface *intf,
data->TermChar = '\n';
/* 2 <= bTag <= 127 USBTMC-USB488 subclass specification 4.3.1 */
data->iin_bTag = 2;
+ data->msgid_out = 1;
+ data->msgid_in = 2;
/* USBTMC devices have only one setting, so use that */
iface_desc = data->intf->cur_altsetting;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usbtmc: vendor specific i/o
On Mon, Aug 01, 2016 at 08:40:02PM +0200, Ladislav Michl wrote: > Hi, > > I need to ask usbtmc device for vendor specific data. What about adding ioctl > which switches MsgIDs to be vendor specific? Or are there other, better, > options? Patch bellow ilustrates changes to allow reading and writing vendor > specific messages. I don't understand exactly what you are changing here, it seems like you are just changing the messages being sent to the device? Doesn't that break existing users? confused, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
