RE: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC
> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Tuesday, June 11, 2019 6:04 PM
> To: Regupathy, Rajaram
> Cc: Mathias Nyman ; Pandey, Prabhat Chand
> ; linux-usb@vger.kernel.org; Nyman,
> Mathias ; K V, Abhilash ;
> Balaji, M
> Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw
> interface on DbC
> Importance: High
>
> On Tue, Jun 11, 2019 at 12:17:40PM +, Regupathy, Rajaram wrote:
> >
> >
> > > -Original Message-
> > > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > > Sent: Tuesday, June 11, 2019 3:23 PM
> > > To: Regupathy, Rajaram
> > > Cc: Mathias Nyman ; Pandey, Prabhat
> > > Chand ; linux-usb@vger.kernel.org;
> > > Nyman, Mathias ; K V, Abhilash
> > > ; Balaji, M
> > > Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to
> > > provide a raw interface on DbC
> > > Importance: High
> > >
> > > On Tue, Jun 11, 2019 at 09:29:23AM +, Regupathy, Rajaram wrote:
> > > >
> > > >
> > > > > -Original Message-
> > > > > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > > > > Sent: Monday, June 10, 2019 7:46 PM
> > > > > To: Mathias Nyman
> > > > > Cc: Pandey, Prabhat Chand ;
> > > > > linux- u...@vger.kernel.org; Nyman, Mathias
> > > > > ; Regupathy, Rajaram
> > > > > ; K V, Abhilash
> > > > > ; Balaji, M
> > > > > Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to
> > > > > provide a raw interface on DbC
> > > > > Importance: High
> > > > >
> > > > > On Mon, Jun 10, 2019 at 04:53:51PM +0300, Mathias Nyman wrote:
> > > > > > On 7.6.2019 17.21, Greg KH wrote:
> > > > > > > On Fri, Jun 07, 2019 at 12:03:05PM +0530, Prabhat Chand Pandey
> wrote:
> > > > > > > > From: Abhilash K V
> > > > > > > >
> > > > > > > > This patch provides a raw device interface on xhci Debug
> > > > > > > > capability.
> > > > > > >
> > > > > > > What is a "raw device"?
> > > > > > >
> > > > > > > > This abstracts dbc functionality to user space inorder to
> > > > > > > > facilitate various frameworks to utilize xhci debug capability.
> > > > > > >
> > > > > > > I do not understand this sentance at all. Please provide a
> > > > > > > lot more information.
> > > > > > >
> > > > > > > > It helps to render the target as an usb debug class device
> > > > > > > > on host and establish an usb connection by providing two bulk
> endpoints.
> > > > > > >
> > > > > > > provide bulk endpoints where? To send data where? This is
> > > > > > > very confusing and does not make any sense to me...
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > [don't dynamically allocate tiny space for name only
> > > > > > > > -Mathias]
> > > > > > > > Signed-off-by: Rajaram Regupathy
> > > > > > > >
> > > > > > > > Signed-off-by: Prabhat Chand Pandey
> > > > > > > >
> > > > > > > > Signed-off-by: Abhilash K V
> > > > > > > > Acked-by: Mathias Nyman
> > > > > > > > ---
> > > > > > ...
> > > > > > >
> > > > > > > So you have a new char device, with a undocumented and
> > > > > > > unknown format of data flowing across it to the device. How
> > > > > > > in the world are we supposed to use this thing? Where is it
> > > > > > > documented? What does it do? How can you use it?
> > > >
> > > > We had captured all information in patch 5/5 patch in the
> > > > documentation
> > > part.
> > > > We could always improve the documentation. Please let us know
> > >
> > > The documentation needs work, see my comments on that.
> > >
> > > Also, I don't think you answered these basic questions there, like
> > > "what is the data format", and "how is this supposed to be used".
> >
> > Sure. dbc_raw("dbcfs" could have been better) is a an interface similar to
> gadget(ffs) or host (usbfs) drivers of USB. These "*fs" provides user land to
> develop class drivers. Thus the transport is agnostic on the data format,
> where
> we could implement an MTP class (ffs,usbfs) or ADB(dbcfs/dbc_raw).
>
> Please wrap your lines at 72 columns...
>
> Anyway, this is not a filesystem interface, you have created a char device.
> Much like the tty char device node you have today, right? Just seems to use
> different ioctls, and requires a custom userspace program.
>
> > But I agree we need to provide additional documentation similar to
> > Documentation/usb/proc_usb_info.txt or
> > Documentation/usb/functionfs.txt
>
> You need some sort of documentation :)
>
> > > > > > > I don't mean to be so harsh here, but come on people, this
> > > > > > > stuff needs a lot more background documentation,
> > > > > > > information, and explaination as to exactly why in the world
> > > > > > > we need any of this, and what it
> > > > > even does!
> > > > > > >
> > > > > > > Also, you need to fix the code, it doesn't work as pointed
> > > > > > > out in a few places :)
> > > > > > >
> > > > > >
> > > > > > Thanks for going through this.
> > > > > > It's now clear this is far from ready.
> > > > > > I need to re-evaluate my position on this, not just the
Re: kernel NULL pointer dereference, ucsi bug
On Tue, Jun 11, 2019 at 10:19:27PM +1000, Vladimir Yerilov wrote:
> Hi Heikki,
>
> > What do you have connected to the USB Type-C connectors on the
> machine when that happens?
>
> Now, on 5.2-rc4, it happens only during boot and when power cord is
> connected to laptop's type-c charging port. Another port, which does
> not support charging, does not cause this problem, I mean I can have
> something connected to it or not, no issue is observed if charger is
> disconnected. I even tried to connect my hub to the guilty charging
> port, everything works fine until hub is powered from the charger, in
> that case the bug happens again. This differs from my previous tests
> when whatever I had connected to the charging port, caused failures,
> even type C to A OTG cable. To summarize: now it happens when power
> cord from the charger is connected during the boot, but if I connect
> it after boot (e.g. after logging in to my X session), no crash
> occurs.
>
> > Can you use the machine normally when nothing is connected to the USB
> > Type-C connectors
>
> Yeah, everything is great when nothing is connected to the faulty
> charging port during the boot, and charger may be connected later, but
> it's kinda... wrong.
>
> > Is it possible to send complete dmesg output after that happened?
>
> You can find everything attached (done with journalctl -k -xb)
Thanks. I don't see anything the driver is doing wrong. I'm attaching
a patch. Can you test if it fixes the issue?
Br,
--
heikki
>From d6979950ee911194dab9ae7513fe36dc3730f7a9 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus
Date: Tue, 11 Jun 2019 11:10:25 +0300
Subject: [PATCH] usb: typec: Make sure an alt mode exist before getting its
partner
Interim. For testing only!
Adding check to typec_altmode_get_partner() to prevent
potential NULL pointer dereference.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
index 76299b6ff06d..74cb3c2ecb34 100644
--- a/drivers/usb/typec/bus.c
+++ b/drivers/usb/typec/bus.c
@@ -192,7 +192,7 @@ EXPORT_SYMBOL_GPL(typec_altmode_vdm);
const struct typec_altmode *
typec_altmode_get_partner(struct typec_altmode *adev)
{
- return &to_altmode(adev)->partner->adev;
+ return adev ? &to_altmode(adev)->partner->adev : NULL;
}
EXPORT_SYMBOL_GPL(typec_altmode_get_partner);
--
2.20.1
Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC
On Wed, Jun 12, 2019 at 08:49:11AM +, Regupathy, Rajaram wrote: > > > > -Original Message- > > From: Greg KH [mailto:gre...@linuxfoundation.org] > > Sent: Tuesday, June 11, 2019 6:04 PM > > To: Regupathy, Rajaram > > Cc: Mathias Nyman ; Pandey, Prabhat Chand > > ; linux-usb@vger.kernel.org; Nyman, > > Mathias ; K V, Abhilash ; > > Balaji, M > > Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a > > raw > > interface on DbC > > Importance: High Please fix your quoting logic, this is never needed in an email. And I don't find these emails of "High" importance :) > > > > > The larger goal here is to have DbC as a unified debug > > > > > infrastructure for > > > > different debug methods like KGDB or early printk and leverage the > > > > benefits of a dedicated debug infra (DbC) brings in. > > > > > > > > Have you modified kgdb for this? Do you have patches for that? > > > No kgdb changes. For kgdb to work we added necessary wrapper functions > > required on the dbc_tty interface which already part of kernel. We have > > functionally verified and shall push the patch subsequently. Am I missing > > something ? > > > > You are missing the justification of a custom api that requires all > > userspace > > tools to be modified instead of using the existing tty api that everything > > "just > > works" with today. > > I was referring to the poll methods required for KGDB/GDB to work which is > missing in dbc_tty driver in the kernel. Again, fix your line-wrapping :( Anyway, just poing kgdb at the different tty device and it should "just work", right? No need to change anything that I can see. If not, please send patches to get that working so people can use that today. > > > > Who can use this interface in the "real world", is it only > > > > developers that have access to the special hardware dongle? Or can > > > > anyone use this on their laptops for getting console access in a way > > > > that is somehow "better" than the existing interface? > > > > > > No special hardware is required. As indicated earlier developers need a > > > USB A- > > A debug cable and anyone can use it to get console access. > > > > Where can I get one of those? > Here is one example: > https://www.amazon.com/SIIG-SuperSpeed-Cable-Meters-CB-US0212-S1/dp/B0032ANCBO Ah, nice! I'll try to see if I can get that in my country... Nope, not available in Europe from what I can tell, I'll have to wait until the next time I'm in the US :( > > > Yes it is better that existing usb-serial converters with each having > > > proprietary > > drivers . This is a plug and play solution providing super speed interface. > > > > I don't understand, what is "proprietary" about the existing tty api? > > It's a generic tty device node, right? What am I missing? > > I am referring to usb-serial controller drivers as in > "drivers/usb/serial/Kconfig" which has vendors configs leading to much of > kernel maintenance. DbC driver would provide necessary functionality without > any of those. Again, line-wrapping... I have no idea what you are referring to "leading to much of kernel maintenance." You don't need to include _all_ usb-serial drivers, just the usb-serial core and the specific driver. No different from any other serial port device, which Linux has supported for decades now. Use the functionality we have already, do not create duplicate code just because you want to, that's a sure way to get your patches rejected. And this _is_ a serial connection to the other device, why pretend that it is not? Again, what is the reason why you can not use what you have today? > > > > And just how much "faster" is all of this than the current tty > > > > interface? What is lacking in the tty interface today that you need > > > > this new, special one? Can you just not fix any bottleneck in the > > > > tty driver if you are not properly saturating the bus? > > > > > > IMHO, tty is a legacy interface designed for the purpose it serves > > > for. Modern High speed IO will not fit into tty framework and > > > refactoring it will not bring any real value. We have captured the > > > initial performance numbers in the documentation patch 5/5. Please > > > correct me if I am missing something > > > > Why will "modern high speed IO" not fit into the tty framework? Where is > > the > > bottleneck? We have tty devices that seem to run at "line speed" > > on a firewire connection today, and that should be faster than whatever this > > host controller can pump out, right? > > Though I am not aware of the design thoughts behind firewire-tty driver which > is in staging, I see > the to do list and git logs indicate buffer over flow issues indicating the > tty framework cannot handle high speed IO. That is that one driver, not yours. > Thus our rationale of why tty may not serve the purpose as indicated below > gets ratified > - Performance & stability ( multiple layers, >1GB file copy CRC errors)
Re: kernel NULL pointer dereference, ucsi bug
Yes, it works. I've built 5.2-rc4 with this patch and it works fine now, the problem is gone. It is great that I didn't have to downgrade BIOS as a last resort in my attempts to workaround this issue. Thank you! Cc to Greg in order to let him know that it is resolved now. ср, 12 июн. 2019 г. в 19:55, Heikki Krogerus : > > On Tue, Jun 11, 2019 at 10:19:27PM +1000, Vladimir Yerilov wrote: > > Hi Heikki, > > > > > What do you have connected to the USB Type-C connectors on the > > machine when that happens? > > > > Now, on 5.2-rc4, it happens only during boot and when power cord is > > connected to laptop's type-c charging port. Another port, which does > > not support charging, does not cause this problem, I mean I can have > > something connected to it or not, no issue is observed if charger is > > disconnected. I even tried to connect my hub to the guilty charging > > port, everything works fine until hub is powered from the charger, in > > that case the bug happens again. This differs from my previous tests > > when whatever I had connected to the charging port, caused failures, > > even type C to A OTG cable. To summarize: now it happens when power > > cord from the charger is connected during the boot, but if I connect > > it after boot (e.g. after logging in to my X session), no crash > > occurs. > > > > > Can you use the machine normally when nothing is connected to the USB > > > Type-C connectors > > > > Yeah, everything is great when nothing is connected to the faulty > > charging port during the boot, and charger may be connected later, but > > it's kinda... wrong. > > > > > Is it possible to send complete dmesg output after that happened? > > > > You can find everything attached (done with journalctl -k -xb) > > Thanks. I don't see anything the driver is doing wrong. I'm attaching > a patch. Can you test if it fixes the issue? > > Br, > > -- > heikki -- Best regards, Vladimir Yerilov
Re: kernel NULL pointer dereference, ucsi bug
On Wed, Jun 12, 2019 at 10:23:56PM +1000, Vladimir Yerilov wrote: > Yes, it works. > I've built 5.2-rc4 with this patch and it works fine now, the problem is gone. > It is great that I didn't have to downgrade BIOS as a last resort in > my attempts to workaround this issue. > > Thank you! > > Cc to Greg in order to let him know that it is resolved now. It is not "resolved" until the fix is merged into Linus's tree :) thanks, greg k-h
Re: kernel NULL pointer dereference, ucsi bug
Oh, you are right. I meant... You know what I meant :) I hope the fix will get there eventually in one way or another. Should you need any further tests from my side, just ask and I will make my faulty machine work on it. Thank you guys again for your kind assistance. ср, 12 июн. 2019 г. в 22:32, Greg KH : > > On Wed, Jun 12, 2019 at 10:23:56PM +1000, Vladimir Yerilov wrote: > > Yes, it works. > > I've built 5.2-rc4 with this patch and it works fine now, the problem is > > gone. > > It is great that I didn't have to downgrade BIOS as a last resort in > > my attempts to workaround this issue. > > > > Thank you! > > > > Cc to Greg in order to let him know that it is resolved now. > > It is not "resolved" until the fix is merged into Linus's tree :) > > thanks, > > greg k-h -- Best regards, Vladimir Yerilov
[PATCH] USB:serial:pl2303:Add new PID to support PL2303HXN (TYPE_HXN)
Prolific has developed a new USB to UART chip: PL2303HXN
PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB
The Vendor request used by the PL2303HXN (TYPE_HXN) is different from
the existing PL2303 series (TYPE_HX & TYPE_01).
Therefore, different Vendor requests are used to issue related commands.
1. Added a new TYPE_HXN type in pl2303_type_data, and then executes
new Vendor request,new flow control and other related instructions
if TYPE_HXN is recognized.
2. Because the new PL2303HXN only accept the new Vendor request,
the old Vendor request cannot be accepted (the error message
will be returned)
So first determine the TYPE_HX or TYPE_HXN through
PL2303_READ_TYPE_HX_STATUS in pl2303_startup.
2.1 If the return message is "1", then the PL2303 is the existing
TYPE_HX/ TYPE_01 series.
The other settings in pl2303_startup are to continue execution.
2.2 If the return message is "not 1", then the PL2303 is the new
TYPE_HXN series.
The other settings in pl2303_startup are ignored.
(PL2303HXN will directly use the default value in the hardware,
no need to add additional settings through the software)
3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
down/up stream used by TYPE_HX.
Therefore, we will also execute different instructions here.
4. In pl2303_set_termios: The UART flow control instructions used by
TYPE_HXN/TYPE_HX/TYPE_01 are different.
Therefore, we will also execute different instructions here.
5. In pl2303_vendor_read & pl2303_vendor_write, since TYPE_HXN is different
from the vendor request instruction used by TYPE_HX/TYPE_01,
it will also execute different instructions here.
6. In pl2303_update_reg: TYPE_HXN used different register for flow control.
Therefore, we will also execute different instructions here.
Signed-off-by: Charles Yeh
---
drivers/usb/serial/pl2303.c | 113 +---
drivers/usb/serial/pl2303.h | 7 ++-
2 files changed, 97 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index 55122ac84518..22ad82aa3894 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -47,6 +47,12 @@ static const struct usb_device_id id_table[] = {
{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_MOTOROLA) },
{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_ZTEK) },
{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_TB) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GC) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GB) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GT) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GL) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GE) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GS) },
{ USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID) },
{ USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID_RSAQ5) },
{ USB_DEVICE(ATEN_VENDOR_ID, ATEN_PRODUCT_ID),
@@ -129,9 +135,11 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define VENDOR_WRITE_REQUEST_TYPE 0x40
#define VENDOR_WRITE_REQUEST 0x01
+#define VENDOR_WRITE_NREQUEST 0x80
#define VENDOR_READ_REQUEST_TYPE 0xc0
#define VENDOR_READ_REQUEST0x01
+#define VENDOR_READ_NREQUEST 0x81
#define UART_STATE_INDEX 8
#define UART_STATE_MSR_MASK0x8b
@@ -146,12 +154,20 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define UART_CTS 0x80
#define PL2303_FLOWCTRL_MASK 0xf0
+#define PL2303_HXN_FLOWCTRL_MASK 0x1C
+#define PL2303_READ_TYPE_HX_STATUS 0x8080
+#define PL2303_HXN_FLOWCTRL0x0A
+#define PL2303_HXN_CTRL_RTS_CTS0x18
+#define PL2303_HXN_CTRL_XON_XOFF 0x0C
+#define PL2303_HXN_CTRL_NONE 0x1C
+#define PL2303_HXN_RESET_DOWN_UPSTREAM 0x07
static void pl2303_set_break(struct usb_serial_port *port, bool enable);
enum pl2303_type {
TYPE_01,/* Type 0 and 1 (difference unknown) */
TYPE_HX,/* HX version of the pl2303 chip */
+ TYPE_HXN, /* HXN version of the pl2303 chip */
TYPE_COUNT
};
@@ -183,16 +199,26 @@ static const struct pl2303_type_data
pl2303_type_data[TYPE_COUNT] = {
[TYPE_HX] = {
.max_baud_rate = 1200,
},
+ [TYPE_HXN] = {
+ .max_baud_rate = 1200,
+ },
};
static int pl2303_vendor_read(struct usb_serial *serial, u16 value,
unsigned char buf[1])
{
struct device *dev = &serial->interface->dev;
+ struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
int res;
+ u8 request;
+
+ if (spriv->type == &pl2303_type_data[TYPE_HXN])
+ request = VENDOR_READ_NREQUEST;
+ else
+
Re: kernel NULL pointer dereference, ucsi bug
On Wed, Jun 12, 2019 at 10:57:35PM +1000, Vladimir Yerilov wrote: > Oh, you are right. I meant... You know what I meant :) > I hope the fix will get there eventually in one way or another. Should > you need any further tests from my side, just ask and I will make my > faulty machine work on it. > > Thank you guys again for your kind assistance. The patch I gave you does not fix the root cause, but instead just works around it. The change it brings is however useful, so I'm going to send the patch to the linux-usb ml as a fix for this issue. I think the root problem in this case is that the firmware is reporting the alternate modes in a customized way. The problem is actually not with the firmware itself. The problem is with UCSI specification. The UCSI spec does not explain how exactly the alternate modes should be handled, so every platform does it a bit differently (a major operating system does not seem to care about the alternate modes). The ucsi driver we have in kernel already considers a number of different ways the alternate modes could be handled, but clearly not the way Kaby Lakes handle them. I'm going to continue debugging this with a Kaby Lake board, and once/if I figure out how to manage the alternate modes on it, I'll send the final fix. But for now, let's workaround the problem. thanks, -- heikki
[PATCH] usb: typec: Make sure an alt mode exist before getting its partner
Adding check to typec_altmode_get_partner() to prevent
potential NULL pointer dereference.
Reported-by: Vladimir Yerilov
Fixes: ad74b8649bea ("usb: typec: ucsi: Preliminary support for alternate
modes")
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
index eda4a45f0d0a..70072e3705ca 100644
--- a/drivers/usb/typec/bus.c
+++ b/drivers/usb/typec/bus.c
@@ -192,7 +192,7 @@ EXPORT_SYMBOL_GPL(typec_altmode_vdm);
const struct typec_altmode *
typec_altmode_get_partner(struct typec_altmode *adev)
{
- return &to_altmode(adev)->partner->adev;
+ return adev ? &to_altmode(adev)->partner->adev : NULL;
}
EXPORT_SYMBOL_GPL(typec_altmode_get_partner);
--
2.20.1
Re: [PATCH] USB:serial:pl2303:Add new PID to support PL2303HXN (TYPE_HXN)
On Wed, Jun 12, 2019 at 09:18:41PM +0800, Charles Yeh wrote: > Prolific has developed a new USB to UART chip: PL2303HXN > PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB > The Vendor request used by the PL2303HXN (TYPE_HXN) is different from > the existing PL2303 series (TYPE_HX & TYPE_01). > Therefore, different Vendor requests are used to issue related commands. > Signed-off-by: Charles Yeh > --- As I've asked you several times already, make sure to - add a patch version to your subject (e.g. [PATCH v5], or which version number you're currently at) - include a changelog here, below the --- line, so we know what changed since previous versions - fix up your subject line (add a space after each colon) Please fix up and resend. Thanks, Johan
