Re: [PATCH] option: Add support for Quectel EP06

[Johan Hovold]

On Sun, Feb 04, 2018 at 07:24:22PM +0100, Bjørn Mork wrote:
> Johan Hovold  writes:

> >> IIRC I considered just dumping the BIT(x) into the .driver_info but
> >> then we'd only have 16 bits for each of send_setup and reserved on 32-
> >> bit arches and I wasn't sure that was enough.  I've seen some devices
> >> with lots of interfaces.  But doing it this way might have been clearer
> >> than a sidecar struct like option_blacklist_info.
> >
> > Yeah, we should probably consider moving over to something like that.
> > 16 bits would at least be enough for the devices we currently have
> > blacklists for.
> 
> Yes, I think the current driver documents pretty well that we don't need
> backlists for high interface numbers.
> 
> Checking the devices I have ever used, the only ones I found with
> interface numbers higher than 16 were the Sierra Wireless modems of the
> MDM9200/MDM9600 generation.  THe used 19 and 20 for two of their
> QMI/RMNET functions.  But they are handled by the qcserial driver, so
> that's not an issue wrt option.
> 
> I say go for the simple bitmasks!

Great, thanks for the feedback.

> You might also consider a general blacklist for stuff like ADB which
> always need blacklisting.  By now, Google owns ff/42/xx whether you like
> it or not.

Good point.

I won't be able to look at this for another few weeks myself, but will
try to come with some fairly compact notation for this unless someone
beats me to it.

Thanks,
Johan
--
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: [PATCH] option: Add support for Quectel EP06

[Bjørn Mork]

Johan Hovold  writes:

> IIRC we have already started reusing entries, but the names have not
> always been made generic in the process. I think Bjørn may have proposed
> a generic naming scheme at some point, but that essentially just meant
> we encoded the two masks in the name.

Maybe. I dont' remember.

> Then we may just move over to
> encoding the masks directly in driver_data instead, at least if 16 bits
> is enough.

Agreed.

>> IIRC I considered just dumping the BIT(x) into the .driver_info but
>> then we'd only have 16 bits for each of send_setup and reserved on 32-
>> bit arches and I wasn't sure that was enough.  I've seen some devices
>> with lots of interfaces.  But doing it this way might have been clearer
>> than a sidecar struct like option_blacklist_info.
>
> Yeah, we should probably consider moving over to something like that.
> 16 bits would at least be enough for the devices we currently have
> blacklists for.

Yes, I think the current driver documents pretty well that we don't need
backlists for high interface numbers.

Checking the devices I have ever used, the only ones I found with
interface numbers higher than 16 were the Sierra Wireless modems of the
MDM9200/MDM9600 generation.  THe used 19 and 20 for two of their
QMI/RMNET functions.  But they are handled by the qcserial driver, so
that's not an issue wrt option.

I say go for the simple bitmasks!

You might also consider a general blacklist for stuff like ADB which
always need blacklisting.  By now, Google owns ff/42/xx whether you like
it or not.


Bjørn
--
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: [PATCH] option: Add support for Quectel EP06

[Johan Hovold]

On Wed, Jan 31, 2018 at 04:35:34PM -0600, Dan Williams wrote:
> On Wed, 2018-01-31 at 16:32 -0600, Dan Williams wrote:
> > On Thu, 2018-02-01 at 09:17 +1100, Johan Hovold wrote:
> > > On Wed, Jan 31, 2018 at 09:56:01AM +0100, Kristian Evensen wrote:
> > > > Hi Johan,
> > > > 
> > > > On Wed, Jan 31, 2018 at 7:38 AM, Johan Hovold 
> > > > wrote:
> > > > > This will probably have to do for now, but we already have
> > > > > another
> > > > > blacklist struct with the same content which we could rename
> > > > > and
> > > > > reuse.
> > > > 
> > > > I noticed the same, but wasn't quite sure about the policy on
> > > > renaming/recycling and added a new blacklist entry. I can rename
> > > > the
> > > > entry and update references as part of this commit. What would be
> > > > an
> > > > appropriate name, something straight-forward like
> > > > "net_intf4_intf5_blacklist"?
> > > 
> > > Yeah, the policy isn't entirely clear to me either. ;) The
> > > net_blacklist
> > > are used to blacklist a single network interface, but here the
> > > other
> > > interface was used for ADB and for the other driver it was for an
> > > audio
> > > interface I think.
> > 
> > When I added/consolidated this feature a long time back I didn't
> > think
> > we'd end up with as many common entries as we have.  I think it's
> > fine
> > to re-use use a common entry, but if you do and the common entry is
> > named after a vendor/model, then make it generic.

IIRC we have already started reusing entries, but the names have not
always been made generic in the process. I think Bjørn may have proposed
a generic naming scheme at some point, but that essentially just meant
we encoded the two masks in the name. Then we may just move over to
encoding the masks directly in driver_data instead, at least if 16 bits
is enough.

> IIRC I considered just dumping the BIT(x) into the .driver_info but
> then we'd only have 16 bits for each of send_setup and reserved on 32-
> bit arches and I wasn't sure that was enough.  I've seen some devices
> with lots of interfaces.  But doing it this way might have been clearer
> than a sidecar struct like option_blacklist_info.

Yeah, we should probably consider moving over to something like that.
16 bits would at least be enough for the devices we currently have
blacklists for.

Thanks,
Johan
--
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: [PATCH] option: Add support for Quectel EP06

[Dan Williams]

On Wed, 2018-01-31 at 16:32 -0600, Dan Williams wrote:
> On Thu, 2018-02-01 at 09:17 +1100, Johan Hovold wrote:
> > On Wed, Jan 31, 2018 at 09:56:01AM +0100, Kristian Evensen wrote:
> > > Hi Johan,
> > > 
> > > On Wed, Jan 31, 2018 at 7:38 AM, Johan Hovold 
> > > wrote:
> > > > This will probably have to do for now, but we already have
> > > > another
> > > > blacklist struct with the same content which we could rename
> > > > and
> > > > reuse.
> > > 
> > > I noticed the same, but wasn't quite sure about the policy on
> > > renaming/recycling and added a new blacklist entry. I can rename
> > > the
> > > entry and update references as part of this commit. What would be
> > > an
> > > appropriate name, something straight-forward like
> > > "net_intf4_intf5_blacklist"?
> > 
> > Yeah, the policy isn't entirely clear to me either. ;) The
> > net_blacklist
> > are used to blacklist a single network interface, but here the
> > other
> > interface was used for ADB and for the other driver it was for an
> > audio
> > interface I think.
> 
> When I added/consolidated this feature a long time back I didn't
> think
> we'd end up with as many common entries as we have.  I think it's
> fine
> to re-use use a common entry, but if you do and the common entry is
> named after a vendor/model, then make it generic.

IIRC I considered just dumping the BIT(x) into the .driver_info but
then we'd only have 16 bits for each of send_setup and reserved on 32-
bit arches and I wasn't sure that was enough.  I've seen some devices
with lots of interfaces.  But doing it this way might have been clearer
than a sidecar struct like option_blacklist_info.

Dan

> Dan
> 
> > You can just add the duplicate entry for now and if this comes up
> > again,
> > we'll figure out a new (naming) policy.
> > 
> > Thanks,
> > Johan
> > --
> > 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
> 
> --
> 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
--
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: [PATCH] option: Add support for Quectel EP06

[Dan Williams]

On Thu, 2018-02-01 at 09:17 +1100, Johan Hovold wrote:
> On Wed, Jan 31, 2018 at 09:56:01AM +0100, Kristian Evensen wrote:
> > Hi Johan,
> > 
> > On Wed, Jan 31, 2018 at 7:38 AM, Johan Hovold 
> > wrote:
> > > This will probably have to do for now, but we already have
> > > another
> > > blacklist struct with the same content which we could rename and
> > > reuse.
> > 
> > I noticed the same, but wasn't quite sure about the policy on
> > renaming/recycling and added a new blacklist entry. I can rename
> > the
> > entry and update references as part of this commit. What would be
> > an
> > appropriate name, something straight-forward like
> > "net_intf4_intf5_blacklist"?
> 
> Yeah, the policy isn't entirely clear to me either. ;) The
> net_blacklist
> are used to blacklist a single network interface, but here the other
> interface was used for ADB and for the other driver it was for an
> audio
> interface I think.

When I added/consolidated this feature a long time back I didn't think
we'd end up with as many common entries as we have.  I think it's fine
to re-use use a common entry, but if you do and the common entry is
named after a vendor/model, then make it generic.

Dan

> You can just add the duplicate entry for now and if this comes up
> again,
> we'll figure out a new (naming) policy.
> 
> Thanks,
> Johan
> --
> 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
--
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: [PATCH] option: Add support for Quectel EP06

[Johan Hovold]

On Wed, Jan 31, 2018 at 09:56:01AM +0100, Kristian Evensen wrote:
> Hi Johan,
> 
> On Wed, Jan 31, 2018 at 7:38 AM, Johan Hovold  wrote:
> > This will probably have to do for now, but we already have another
> > blacklist struct with the same content which we could rename and
> > reuse.
> 
> I noticed the same, but wasn't quite sure about the policy on
> renaming/recycling and added a new blacklist entry. I can rename the
> entry and update references as part of this commit. What would be an
> appropriate name, something straight-forward like
> "net_intf4_intf5_blacklist"?

Yeah, the policy isn't entirely clear to me either. ;) The net_blacklist
are used to blacklist a single network interface, but here the other
interface was used for ADB and for the other driver it was for an audio
interface I think.

You can just add the duplicate entry for now and if this comes up again,
we'll figure out a new (naming) policy.

Thanks,
Johan
--
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: [PATCH] option: Add support for Quectel EP06

[Kristian Evensen]

Hi Johan,

On Wed, Jan 31, 2018 at 7:38 AM, Johan Hovold  wrote:
> This will probably have to do for now, but we already have another
> blacklist struct with the same content which we could rename and
> reuse.

I noticed the same, but wasn't quite sure about the policy on
renaming/recycling and added a new blacklist entry. I can rename the
entry and update references as part of this commit. What would be an
appropriate name, something straight-forward like
"net_intf4_intf5_blacklist"?

BR,
Kristian
--
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: [PATCH] option: Add support for Quectel EP06

[Johan Hovold]

On Tue, Jan 30, 2018 at 03:06:38PM +0100, Kristian Evensen wrote:
> The Quectel EP06 is a Cat. 6 LTE modem, and the interface mapping is as
> follows:
> 
> 0: Diag
> 1: NMEA
> 2: AT
> 3: Modem
> 
> Interface 4 is QMI and interface 5 is ADB, so they are blacklisted.
> 
> Signed-off-by: Kristian Evensen 
> ---
>  drivers/usb/serial/option.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> index b6320e3be429..2d53e14ff93b 100644
> --- a/drivers/usb/serial/option.c
> +++ b/drivers/usb/serial/option.c
> @@ -241,6 +241,7 @@ static void option_instat_callback(struct urb *urb);
>  #define QUECTEL_PRODUCT_EC21 0x0121
>  #define QUECTEL_PRODUCT_EC25 0x0125
>  #define QUECTEL_PRODUCT_BG96 0x0296
> +#define QUECTEL_PRODUCT_EP06 0x0306
>  
>  #define CMOTECH_VENDOR_ID0x16d8
>  #define CMOTECH_PRODUCT_6001 0x6001
> @@ -686,6 +687,10 @@ static const struct option_blacklist_info 
> yuga_clm920_nc5_blacklist = {
>   .reserved = BIT(1) | BIT(4),
>  };
>  
> +static const struct option_blacklist_info quectel_ep06_blacklist = {
> + .reserved = BIT(4) | BIT(5),
> +};

This will probably have to do for now, but we already have another
blacklist struct with the same content which we could rename and
reuse.

Can you please just fix up the commit-summary prefix (use "USB: serial:
option: ...") and resend as a v2?

Thanks,
Johan
--
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

[PATCH] option: Add support for Quectel EP06

[Kristian Evensen]

The Quectel EP06 is a Cat. 6 LTE modem, and the interface mapping is as
follows:

0: Diag
1: NMEA
2: AT
3: Modem

Interface 4 is QMI and interface 5 is ADB, so they are blacklisted.

Signed-off-by: Kristian Evensen 
---
 drivers/usb/serial/option.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index b6320e3be429..2d53e14ff93b 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -241,6 +241,7 @@ static void option_instat_callback(struct urb *urb);
 #define QUECTEL_PRODUCT_EC21   0x0121
 #define QUECTEL_PRODUCT_EC25   0x0125
 #define QUECTEL_PRODUCT_BG96   0x0296
+#define QUECTEL_PRODUCT_EP06   0x0306
 
 #define CMOTECH_VENDOR_ID  0x16d8
 #define CMOTECH_PRODUCT_6001   0x6001
@@ -686,6 +687,10 @@ static const struct option_blacklist_info 
yuga_clm920_nc5_blacklist = {
.reserved = BIT(1) | BIT(4),
 };
 
+static const struct option_blacklist_info quectel_ep06_blacklist = {
+   .reserved = BIT(4) | BIT(5),
+};
+
 static const struct usb_device_id option_ids[] = {
{ USB_DEVICE(OPTION_VENDOR_ID, OPTION_PRODUCT_COLT) },
{ USB_DEVICE(OPTION_VENDOR_ID, OPTION_PRODUCT_RICOLA) },
@@ -1200,6 +1205,8 @@ static const struct usb_device_id option_ids[] = {
  .driver_info = (kernel_ulong_t)_intf4_blacklist },
{ USB_DEVICE(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_BG96),
  .driver_info = (kernel_ulong_t)_intf4_blacklist },
+   { USB_DEVICE(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_EP06),
+ .driver_info = (kernel_ulong_t)_ep06_blacklist },
{ USB_DEVICE(CMOTECH_VENDOR_ID, CMOTECH_PRODUCT_6001) },
{ USB_DEVICE(CMOTECH_VENDOR_ID, CMOTECH_PRODUCT_CMU_300) },
{ USB_DEVICE(CMOTECH_VENDOR_ID, CMOTECH_PRODUCT_6003),
-- 
2.14.1

--
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