ALSA: line6: Claim POD X3 USB data interface
diff mbox

Message ID 1479556889-17601-1-git-send-email-dev@andree.sk
State New
Headers show

Commit Message

Andrej Krutak Nov. 19, 2016, 12:01 p.m. UTC
Deny userspace from using the interface concurrently.

Signed-off-by: Andrej Krutak <dev@andree.sk>
---
 sound/usb/line6/driver.h |  1 +
 sound/usb/line6/podhd.c  | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

Comments

Takashi Iwai Nov. 21, 2016, 10:47 a.m. UTC | #1
On Sat, 19 Nov 2016 13:01:29 +0100,
Andrej Krutak wrote:
> 
> Deny userspace from using the interface concurrently.

Could you clarify more what's wrong and what you're changing?


thanks,

Takashi

> 
> Signed-off-by: Andrej Krutak <dev@andree.sk>
> ---
>  sound/usb/line6/driver.h |  1 +
>  sound/usb/line6/podhd.c  | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h
> index 7e3a3aa..5d18957 100644
> --- a/sound/usb/line6/driver.h
> +++ b/sound/usb/line6/driver.h
> @@ -98,6 +98,7 @@ struct line6_properties {
>  
>  	int altsetting;
>  
> +	unsigned ctrl_if;
>  	unsigned ep_ctrl_r;
>  	unsigned ep_ctrl_w;
>  	unsigned ep_audio_r;
> diff --git a/sound/usb/line6/podhd.c b/sound/usb/line6/podhd.c
> index 49cd4a6..6c53b87 100644
> --- a/sound/usb/line6/podhd.c
> +++ b/sound/usb/line6/podhd.c
> @@ -153,6 +153,7 @@ struct usb_line6_podhd {
>  			    .rats = &podhd_ratden},
>  	.bytes_per_channel = 3 /* SNDRV_PCM_FMTBIT_S24_3LE */
>  };
> +static struct usb_driver podhd_driver;
>  
>  static void podhd_startup_start_workqueue(unsigned long data);
>  static void podhd_startup_workqueue(struct work_struct *work);
> @@ -291,8 +292,13 @@ static void podhd_disconnect(struct usb_line6 *line6)
>  	struct usb_line6_podhd *pod = (struct usb_line6_podhd *)line6;
>  
>  	if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) {
> +		struct usb_interface *intf;
> +
>  		del_timer_sync(&pod->startup_timer);
>  		cancel_work_sync(&pod->startup_work);
> +
> +		intf = usb_ifnum_to_if(line6->usbdev, pod->line6.properties->ctrl_if);
> +		usb_driver_release_interface(&podhd_driver, intf);
>  	}
>  }
>  
> @@ -304,10 +310,26 @@ static int podhd_init(struct usb_line6 *line6,
>  {
>  	int err;
>  	struct usb_line6_podhd *pod = (struct usb_line6_podhd *) line6;
> +	struct usb_interface *intf;
>  
>  	line6->disconnect = podhd_disconnect;
>  
>  	if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) {
> +		/* claim the data interface */
> +		intf = usb_ifnum_to_if(line6->usbdev, pod->line6.properties->ctrl_if);
> +		if (!intf) {
> +			dev_err(pod->line6.ifcdev, "interface %d not found\n",
> +			        pod->line6.properties->ctrl_if);
> +			return -ENODEV;
> +		}
> +
> +		err = usb_driver_claim_interface(&podhd_driver, intf, NULL);
> +		if (err != 0) {
> +			dev_err(pod->line6.ifcdev, "can't claim interface %d, error %d\n",
> +			        pod->line6.properties->ctrl_if, err);
> +			return err;
> +		}
> +
>  		/* create sysfs entries: */
>  		err = snd_card_add_dev_attr(line6->card, &podhd_dev_attr_group);
>  		if (err < 0)
> @@ -406,6 +428,7 @@ static int podhd_init(struct usb_line6 *line6,
>  		.altsetting = 1,
>  		.ep_ctrl_r = 0x81,
>  		.ep_ctrl_w = 0x01,
> +		.ctrl_if = 1,
>  		.ep_audio_r = 0x86,
>  		.ep_audio_w = 0x02,
>  	},
> @@ -417,6 +440,7 @@ static int podhd_init(struct usb_line6 *line6,
>  		.altsetting = 1,
>  		.ep_ctrl_r = 0x81,
>  		.ep_ctrl_w = 0x01,
> +		.ctrl_if = 1,
>  		.ep_audio_r = 0x86,
>  		.ep_audio_w = 0x02,
>  	},
> -- 
> 1.9.1
> 
>
Andrej Krutak Nov. 21, 2016, 11:52 a.m. UTC | #2
On Mon, Nov 21, 2016 at 11:47 AM, Takashi Iwai <tiwai@suse.de> wrote:
> On Sat, 19 Nov 2016 13:01:29 +0100,
> Andrej Krutak wrote:
>>
>> Deny userspace from using the interface concurrently.
>
> Could you clarify more what's wrong and what you're changing?
>
>

Userspace apps have to claim USB interfaces before using endpoints in
them (drivers/usb/core/devio.c:checkintf()). It's a lock mechanism so
that two "drivers"
don't steal data from each other. Kernel drivers don't necessarily
have to claim interfaces (obviously, because it worked so far) - but
they should, to lock out userspace...

Do you want that info in a v2 patch?

>>
>> Signed-off-by: Andrej Krutak <dev@andree.sk>
>> ---
>>  sound/usb/line6/driver.h |  1 +
>>  sound/usb/line6/podhd.c  | 24 ++++++++++++++++++++++++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h
>> index 7e3a3aa..5d18957 100644
>> --- a/sound/usb/line6/driver.h
>> +++ b/sound/usb/line6/driver.h
>> @@ -98,6 +98,7 @@ struct line6_properties {
>>
>>       int altsetting;
>>
>> +     unsigned ctrl_if;
>>       unsigned ep_ctrl_r;
>>       unsigned ep_ctrl_w;
>>       unsigned ep_audio_r;
>> diff --git a/sound/usb/line6/podhd.c b/sound/usb/line6/podhd.c
>> index 49cd4a6..6c53b87 100644
>> --- a/sound/usb/line6/podhd.c
>> +++ b/sound/usb/line6/podhd.c
>> @@ -153,6 +153,7 @@ struct usb_line6_podhd {
>>                           .rats = &podhd_ratden},
>>       .bytes_per_channel = 3 /* SNDRV_PCM_FMTBIT_S24_3LE */
>>  };
>> +static struct usb_driver podhd_driver;
>>
>>  static void podhd_startup_start_workqueue(unsigned long data);
>>  static void podhd_startup_workqueue(struct work_struct *work);
>> @@ -291,8 +292,13 @@ static void podhd_disconnect(struct usb_line6 *line6)
>>       struct usb_line6_podhd *pod = (struct usb_line6_podhd *)line6;
>>
>>       if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) {
>> +             struct usb_interface *intf;
>> +
>>               del_timer_sync(&pod->startup_timer);
>>               cancel_work_sync(&pod->startup_work);
>> +
>> +             intf = usb_ifnum_to_if(line6->usbdev, pod->line6.properties->ctrl_if);
>> +             usb_driver_release_interface(&podhd_driver, intf);
>>       }
>>  }
>>
>> @@ -304,10 +310,26 @@ static int podhd_init(struct usb_line6 *line6,
>>  {
>>       int err;
>>       struct usb_line6_podhd *pod = (struct usb_line6_podhd *) line6;
>> +     struct usb_interface *intf;
>>
>>       line6->disconnect = podhd_disconnect;
>>
>>       if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) {
>> +             /* claim the data interface */
>> +             intf = usb_ifnum_to_if(line6->usbdev, pod->line6.properties->ctrl_if);
>> +             if (!intf) {
>> +                     dev_err(pod->line6.ifcdev, "interface %d not found\n",
>> +                             pod->line6.properties->ctrl_if);
>> +                     return -ENODEV;
>> +             }
>> +
>> +             err = usb_driver_claim_interface(&podhd_driver, intf, NULL);
>> +             if (err != 0) {
>> +                     dev_err(pod->line6.ifcdev, "can't claim interface %d, error %d\n",
>> +                             pod->line6.properties->ctrl_if, err);
>> +                     return err;
>> +             }
>> +
>>               /* create sysfs entries: */
>>               err = snd_card_add_dev_attr(line6->card, &podhd_dev_attr_group);
>>               if (err < 0)
>> @@ -406,6 +428,7 @@ static int podhd_init(struct usb_line6 *line6,
>>               .altsetting = 1,
>>               .ep_ctrl_r = 0x81,
>>               .ep_ctrl_w = 0x01,
>> +             .ctrl_if = 1,
>>               .ep_audio_r = 0x86,
>>               .ep_audio_w = 0x02,
>>       },
>> @@ -417,6 +440,7 @@ static int podhd_init(struct usb_line6 *line6,
>>               .altsetting = 1,
>>               .ep_ctrl_r = 0x81,
>>               .ep_ctrl_w = 0x01,
>> +             .ctrl_if = 1,
>>               .ep_audio_r = 0x86,
>>               .ep_audio_w = 0x02,
>>       },
>> --
>> 1.9.1
>>
>>
Takashi Iwai Nov. 21, 2016, 12:24 p.m. UTC | #3
On Mon, 21 Nov 2016 12:52:47 +0100,
Andrej Kruták wrote:
> 
> On Mon, Nov 21, 2016 at 11:47 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Sat, 19 Nov 2016 13:01:29 +0100,
> > Andrej Krutak wrote:
> >>
> >> Deny userspace from using the interface concurrently.
> >
> > Could you clarify more what's wrong and what you're changing?
> >
> >
> 
> Userspace apps have to claim USB interfaces before using endpoints in
> them (drivers/usb/core/devio.c:checkintf()). It's a lock mechanism so
> that two "drivers"
> don't steal data from each other. Kernel drivers don't necessarily
> have to claim interfaces (obviously, because it worked so far) - but
> they should, to lock out userspace...
> 
> Do you want that info in a v2 patch?

Sure, such vital information must be given to the patch.


thanks,

Takashi

> 
> >>
> >> Signed-off-by: Andrej Krutak <dev@andree.sk>
> >> ---
> >>  sound/usb/line6/driver.h |  1 +
> >>  sound/usb/line6/podhd.c  | 24 ++++++++++++++++++++++++
> >>  2 files changed, 25 insertions(+)
> >>
> >> diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h
> >> index 7e3a3aa..5d18957 100644
> >> --- a/sound/usb/line6/driver.h
> >> +++ b/sound/usb/line6/driver.h
> >> @@ -98,6 +98,7 @@ struct line6_properties {
> >>
> >>       int altsetting;
> >>
> >> +     unsigned ctrl_if;
> >>       unsigned ep_ctrl_r;
> >>       unsigned ep_ctrl_w;
> >>       unsigned ep_audio_r;
> >> diff --git a/sound/usb/line6/podhd.c b/sound/usb/line6/podhd.c
> >> index 49cd4a6..6c53b87 100644
> >> --- a/sound/usb/line6/podhd.c
> >> +++ b/sound/usb/line6/podhd.c
> >> @@ -153,6 +153,7 @@ struct usb_line6_podhd {
> >>                           .rats = &podhd_ratden},
> >>       .bytes_per_channel = 3 /* SNDRV_PCM_FMTBIT_S24_3LE */
> >>  };
> >> +static struct usb_driver podhd_driver;
> >>
> >>  static void podhd_startup_start_workqueue(unsigned long data);
> >>  static void podhd_startup_workqueue(struct work_struct *work);
> >> @@ -291,8 +292,13 @@ static void podhd_disconnect(struct usb_line6 *line6)
> >>       struct usb_line6_podhd *pod = (struct usb_line6_podhd *)line6;
> >>
> >>       if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) {
> >> +             struct usb_interface *intf;
> >> +
> >>               del_timer_sync(&pod->startup_timer);
> >>               cancel_work_sync(&pod->startup_work);
> >> +
> >> +             intf = usb_ifnum_to_if(line6->usbdev, pod->line6.properties->ctrl_if);
> >> +             usb_driver_release_interface(&podhd_driver, intf);
> >>       }
> >>  }
> >>
> >> @@ -304,10 +310,26 @@ static int podhd_init(struct usb_line6 *line6,
> >>  {
> >>       int err;
> >>       struct usb_line6_podhd *pod = (struct usb_line6_podhd *) line6;
> >> +     struct usb_interface *intf;
> >>
> >>       line6->disconnect = podhd_disconnect;
> >>
> >>       if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) {
> >> +             /* claim the data interface */
> >> +             intf = usb_ifnum_to_if(line6->usbdev, pod->line6.properties->ctrl_if);
> >> +             if (!intf) {
> >> +                     dev_err(pod->line6.ifcdev, "interface %d not found\n",
> >> +                             pod->line6.properties->ctrl_if);
> >> +                     return -ENODEV;
> >> +             }
> >> +
> >> +             err = usb_driver_claim_interface(&podhd_driver, intf, NULL);
> >> +             if (err != 0) {
> >> +                     dev_err(pod->line6.ifcdev, "can't claim interface %d, error %d\n",
> >> +                             pod->line6.properties->ctrl_if, err);
> >> +                     return err;
> >> +             }
> >> +
> >>               /* create sysfs entries: */
> >>               err = snd_card_add_dev_attr(line6->card, &podhd_dev_attr_group);
> >>               if (err < 0)
> >> @@ -406,6 +428,7 @@ static int podhd_init(struct usb_line6 *line6,
> >>               .altsetting = 1,
> >>               .ep_ctrl_r = 0x81,
> >>               .ep_ctrl_w = 0x01,
> >> +             .ctrl_if = 1,
> >>               .ep_audio_r = 0x86,
> >>               .ep_audio_w = 0x02,
> >>       },
> >> @@ -417,6 +440,7 @@ static int podhd_init(struct usb_line6 *line6,
> >>               .altsetting = 1,
> >>               .ep_ctrl_r = 0x81,
> >>               .ep_ctrl_w = 0x01,
> >> +             .ctrl_if = 1,
> >>               .ep_audio_r = 0x86,
> >>               .ep_audio_w = 0x02,
> >>       },
> >> --
> >> 1.9.1
> >>
> >>
>

Patch
diff mbox

diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h
index 7e3a3aa..5d18957 100644
--- a/sound/usb/line6/driver.h
+++ b/sound/usb/line6/driver.h
@@ -98,6 +98,7 @@  struct line6_properties {
 
 	int altsetting;
 
+	unsigned ctrl_if;
 	unsigned ep_ctrl_r;
 	unsigned ep_ctrl_w;
 	unsigned ep_audio_r;
diff --git a/sound/usb/line6/podhd.c b/sound/usb/line6/podhd.c
index 49cd4a6..6c53b87 100644
--- a/sound/usb/line6/podhd.c
+++ b/sound/usb/line6/podhd.c
@@ -153,6 +153,7 @@  struct usb_line6_podhd {
 			    .rats = &podhd_ratden},
 	.bytes_per_channel = 3 /* SNDRV_PCM_FMTBIT_S24_3LE */
 };
+static struct usb_driver podhd_driver;
 
 static void podhd_startup_start_workqueue(unsigned long data);
 static void podhd_startup_workqueue(struct work_struct *work);
@@ -291,8 +292,13 @@  static void podhd_disconnect(struct usb_line6 *line6)
 	struct usb_line6_podhd *pod = (struct usb_line6_podhd *)line6;
 
 	if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) {
+		struct usb_interface *intf;
+
 		del_timer_sync(&pod->startup_timer);
 		cancel_work_sync(&pod->startup_work);
+
+		intf = usb_ifnum_to_if(line6->usbdev, pod->line6.properties->ctrl_if);
+		usb_driver_release_interface(&podhd_driver, intf);
 	}
 }
 
@@ -304,10 +310,26 @@  static int podhd_init(struct usb_line6 *line6,
 {
 	int err;
 	struct usb_line6_podhd *pod = (struct usb_line6_podhd *) line6;
+	struct usb_interface *intf;
 
 	line6->disconnect = podhd_disconnect;
 
 	if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) {
+		/* claim the data interface */
+		intf = usb_ifnum_to_if(line6->usbdev, pod->line6.properties->ctrl_if);
+		if (!intf) {
+			dev_err(pod->line6.ifcdev, "interface %d not found\n",
+			        pod->line6.properties->ctrl_if);
+			return -ENODEV;
+		}
+
+		err = usb_driver_claim_interface(&podhd_driver, intf, NULL);
+		if (err != 0) {
+			dev_err(pod->line6.ifcdev, "can't claim interface %d, error %d\n",
+			        pod->line6.properties->ctrl_if, err);
+			return err;
+		}
+
 		/* create sysfs entries: */
 		err = snd_card_add_dev_attr(line6->card, &podhd_dev_attr_group);
 		if (err < 0)
@@ -406,6 +428,7 @@  static int podhd_init(struct usb_line6 *line6,
 		.altsetting = 1,
 		.ep_ctrl_r = 0x81,
 		.ep_ctrl_w = 0x01,
+		.ctrl_if = 1,
 		.ep_audio_r = 0x86,
 		.ep_audio_w = 0x02,
 	},
@@ -417,6 +440,7 @@  static int podhd_init(struct usb_line6 *line6,
 		.altsetting = 1,
 		.ep_ctrl_r = 0x81,
 		.ep_ctrl_w = 0x01,
+		.ctrl_if = 1,
 		.ep_audio_r = 0x86,
 		.ep_audio_w = 0x02,
 	},