Re: [PATCH] net: usb: qmi_wwan: added support for Thales Cinterion PLSx3 modem family

Reinhard Speyerer [off-list ref] writes:

quoted

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index af19513a9f75..262d19439b34 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c

@@ -1302,6 +1302,8 @@ static const struct usb_device_id products[] = {
 	{QMI_FIXED_INTF(0x0b3c, 0xc00a, 6)},	/* Olivetti Olicard 160 */
 	{QMI_FIXED_INTF(0x0b3c, 0xc00b, 4)},	/* Olivetti Olicard 500 */
 	{QMI_FIXED_INTF(0x1e2d, 0x0060, 4)},	/* Cinterion PLxx */
+	{QMI_FIXED_INTF(0x1e2d, 0x006f, 8)},	/* Cinterion PLS83/PLS63 */
+	{QMI_QUIRK_SET_DTR(0x1e2d, 0x006f, 8)},
 	{QMI_FIXED_INTF(0x1e2d, 0x0053, 4)},	/* Cinterion PHxx,PXxx */
 	{QMI_FIXED_INTF(0x1e2d, 0x0063, 10)},	/* Cinterion ALASxx (1 RmNet) */
 	{QMI_FIXED_INTF(0x1e2d, 0x0082, 4)},	/* Cinterion PHxx,PXxx (2 RmNet) */

Hi Giacinto,

AFAIK the {QMI_FIXED_INTF(0x1e2d, 0x006f, 8)} is redundant and can simply
be deleted. Please see also commit 14cf4a771b3098e431d2677e3533bdd962e478d8
("drivers: net: usb: qmi_wwan: add QMI_QUIRK_SET_DTR for Telit PID 0x1201")
and commit 97dc47a1308a3af46a09b1546cfb869f2e382a81
("qmi_wwan: apply SET_DTR quirk to Sierra WP7607") for the corresponding
examples from other UE vendors.

Yup, please fix and send a v2.  And please use get_maintainer.pl to get
the proper destinations.

Thanks for spotting this, Reinhard.  I would never have caught it with
my current netdev reading frequency....




Bjørn

Giacinto Cifelli [off-list ref] writes:

Hi Bjørn,
I have fixed and resent, but from your comment I might not have
selected the right line from maintaner.pl?
what I have is this:
$ ./scripts/get_maintainer.pl --file drivers/net/usb/qmi_wwan.c
"Bjørn Mork" [off-list ref] (maintainer:USB QMI WWAN NETWORK DRIVER)
"David S. Miller" [off-list ref] (maintainer:NETWORKING DRIVERS)
Jakub Kicinski [off-list ref] (maintainer:NETWORKING DRIVERS)
netdev@vger.kernel.org (open list:USB QMI WWAN NETWORK DRIVER)
<<<< this seems the right one
linux-usb@vger.kernel.org (open list:USB NETWORKING DRIVERS)
linux-kernel@vger.kernel.org (open list)

I have at the same time sent a patch for another enumeration of the
same product, for cdc_ether.  In that case, I have picked the
following line, which also looked the best fit:
  linux-usb@vger.kernel.org (open list:USB CDC ETHERNET DRIVER)

Did I misinterpret the results of the script?

Yes, but I'll be the first to admit that it isn't easy.  

netdev is definitely correct, and the most important one.

But in theory you are supposed to use all the listed addresses.  Except
that I don't think you need to CC David (and Jakub?) since they probably
read everything in netdev anyway.  And I believe many (most?) people
leave out the linux-kernel catch-all, since it doesn't provide any extra
coverage for networking. At least I do.

Then there's the two remaining addresses.  The linux-usb list is
traditionally CCed on patches touching USB drivers, since the USB
experts are there and not necessarily in netdev.  And I'd like a copy
because that's the only way I'll be able to catch these patches.  I
don't read any of the lists regularily.

This is my interpretation only.  I am sure there are other opinions. But
as usual, you cannot do anything wrong. The worst that can ever happen
is that you have to resend a patch or miss my review of it ;-)


Bjørn

Hi Bjørn,

On Mon, Jan 18, 2021 at 10:02 PM Bjørn Mork [off-list ref] wrote:

Reinhard Speyerer [off-list ref] writes:

quoted

quoted

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index af19513a9f75..262d19439b34 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c

@@ -1302,6 +1302,8 @@ static const struct usb_device_id products[] = {
     {QMI_FIXED_INTF(0x0b3c, 0xc00a, 6)},    /* Olivetti Olicard 160 */
     {QMI_FIXED_INTF(0x0b3c, 0xc00b, 4)},    /* Olivetti Olicard 500 */
     {QMI_FIXED_INTF(0x1e2d, 0x0060, 4)},    /* Cinterion PLxx */
+    {QMI_FIXED_INTF(0x1e2d, 0x006f, 8)},    /* Cinterion PLS83/PLS63 */
+    {QMI_QUIRK_SET_DTR(0x1e2d, 0x006f, 8)},
     {QMI_FIXED_INTF(0x1e2d, 0x0053, 4)},    /* Cinterion PHxx,PXxx */
     {QMI_FIXED_INTF(0x1e2d, 0x0063, 10)},   /* Cinterion ALASxx (1 RmNet) */
     {QMI_FIXED_INTF(0x1e2d, 0x0082, 4)},    /* Cinterion PHxx,PXxx (2 RmNet) */

Hi Giacinto,

AFAIK the {QMI_FIXED_INTF(0x1e2d, 0x006f, 8)} is redundant and can simply
be deleted. Please see also commit 14cf4a771b3098e431d2677e3533bdd962e478d8
("drivers: net: usb: qmi_wwan: add QMI_QUIRK_SET_DTR for Telit PID 0x1201")
and commit 97dc47a1308a3af46a09b1546cfb869f2e382a81
("qmi_wwan: apply SET_DTR quirk to Sierra WP7607") for the corresponding
examples from other UE vendors.

Yup, please fix and send a v2.  And please use get_maintainer.pl to get
the proper destinations.

I have fixed and resent, but from your comment I might not have
selected the right line from maintaner.pl?
what I have is this:
$ ./scripts/get_maintainer.pl --file drivers/net/usb/qmi_wwan.c
"Bjørn Mork" [off-list ref] (maintainer:USB QMI WWAN NETWORK DRIVER)
"David S. Miller" [off-list ref] (maintainer:NETWORKING DRIVERS)
Jakub Kicinski [off-list ref] (maintainer:NETWORKING DRIVERS)
netdev@vger.kernel.org (open list:USB QMI WWAN NETWORK DRIVER)
<<<< this seems the right one
linux-usb@vger.kernel.org (open list:USB NETWORKING DRIVERS)
linux-kernel@vger.kernel.org (open list)

I have at the same time sent a patch for another enumeration of the
same product, for cdc_ether.  In that case, I have picked the
following line, which also looked the best fit:
  linux-usb@vger.kernel.org (open list:USB CDC ETHERNET DRIVER)

Did I misinterpret the results of the script?

Thank you,
Giacinto

Hi Bjørn,


On Wed, Jan 20, 2021 at 2:13 PM Bjørn Mork [off-list ref] wrote:

Giacinto Cifelli [off-list ref] writes:

quoted

Hi Bjørn,
I have fixed and resent, but from your comment I might not have
selected the right line from maintaner.pl?
what I have is this:
$ ./scripts/get_maintainer.pl --file drivers/net/usb/qmi_wwan.c
"Bjørn Mork" [off-list ref] (maintainer:USB QMI WWAN NETWORK DRIVER)
"David S. Miller" [off-list ref] (maintainer:NETWORKING DRIVERS)
Jakub Kicinski [off-list ref] (maintainer:NETWORKING DRIVERS)
netdev@vger.kernel.org (open list:USB QMI WWAN NETWORK DRIVER)
<<<< this seems the right one
linux-usb@vger.kernel.org (open list:USB NETWORKING DRIVERS)
linux-kernel@vger.kernel.org (open list)

I have at the same time sent a patch for another enumeration of the
same product, for cdc_ether.  In that case, I have picked the
following line, which also looked the best fit:
  linux-usb@vger.kernel.org (open list:USB CDC ETHERNET DRIVER)

Did I misinterpret the results of the script?

Yes, but I'll be the first to admit that it isn't easy.

netdev is definitely correct, and the most important one.

But in theory you are supposed to use all the listed addresses.  Except
that I don't think you need to CC David (and Jakub?) since they probably
read everything in netdev anyway.  And I believe many (most?) people
leave out the linux-kernel catch-all, since it doesn't provide any extra
coverage for networking. At least I do.

Then there's the two remaining addresses.  The linux-usb list is
traditionally CCed on patches touching USB drivers, since the USB
experts are there and not necessarily in netdev.  And I'd like a copy
because that's the only way I'll be able to catch these patches.  I
don't read any of the lists regularily.

This is my interpretation only.  I am sure there are other opinions. But
as usual, you cannot do anything wrong. The worst that can ever happen
is that you have to resend a patch or miss my review of it ;-)

looks like "welcome to the maze" :D

So, the letter of the instructions would be send all, but up to you to
leave some of them out.
Got it.
I am going to wait a couple of days on the off chance that my patches
are reviewed, then I will resend.

Thank you and regards,
Giacinto

On Wed, 20 Jan 2021 15:37:59 +0100 Giacinto Cifelli wrote:

On Wed, Jan 20, 2021 at 2:13 PM Bjørn Mork [off-list ref] wrote:

quoted

Giacinto Cifelli [off-list ref] writes:

quoted

Hi Bjørn,
I have fixed and resent, but from your comment I might not have
selected the right line from maintaner.pl?
what I have is this:
$ ./scripts/get_maintainer.pl --file drivers/net/usb/qmi_wwan.c

I always run it on the patch file formatted by git format-patch.
That way it will also make sure to list people relevant to Fixes 
tags etc.

quoted

quoted

"Bjørn Mork" [off-list ref] (maintainer:USB QMI WWAN NETWORK DRIVER)
"David S. Miller" [off-list ref] (maintainer:NETWORKING DRIVERS)
Jakub Kicinski [off-list ref] (maintainer:NETWORKING DRIVERS)
netdev@vger.kernel.org (open list:USB QMI WWAN NETWORK DRIVER)
<<<< this seems the right one
linux-usb@vger.kernel.org (open list:USB NETWORKING DRIVERS)
linux-kernel@vger.kernel.org (open list)

I have at the same time sent a patch for another enumeration of the
same product, for cdc_ether.  In that case, I have picked the
following line, which also looked the best fit:
  linux-usb@vger.kernel.org (open list:USB CDC ETHERNET DRIVER)

Did I misinterpret the results of the script?  

Yes, but I'll be the first to admit that it isn't easy.

netdev is definitely correct, and the most important one.

But in theory you are supposed to use all the listed addresses.  Except
that I don't think you need to CC David (and Jakub?) since they probably
read everything in netdev anyway.  

That's fair, I've even said in the past that folks can skip CCing me.
That said with vger being flaky lately maybe it's not a bad idea after
all to CC maintainers - in case someone objects to the patch, and we
don't see it because some email deity decided to hold onto the message..

quoted

And I believe many (most?) people
leave out the linux-kernel catch-all, since it doesn't provide any extra
coverage for networking. At least I do.

Same, I wish get_maintainers didn't list it :/

quoted

Then there's the two remaining addresses.  The linux-usb list is
traditionally CCed on patches touching USB drivers, since the USB
experts are there and not necessarily in netdev.  And I'd like a copy
because that's the only way I'll be able to catch these patches.  I
don't read any of the lists regularily.

This is my interpretation only.  I am sure there are other opinions. But
as usual, you cannot do anything wrong. The worst that can ever happen
is that you have to resend a patch or miss my review of it ;-)
 

looks like "welcome to the maze" :D

So, the letter of the instructions would be send all, but up to you to
leave some of them out.
Got it.
I am going to wait a couple of days on the off chance that my patches
are reviewed, then I will resend.

Thank you and regards,
Giacinto