Re: [Intel-gfx] [PATCH 1/3] treewide: Lift switch variables out of switches
On Thu, Jan 24, 2019 at 07:55:51AM +1300, Kees Cook wrote:
> On Thu, Jan 24, 2019 at 4:44 AM Jani Nikula
> wrote:
> >
> > On Wed, 23 Jan 2019, Edwin Zimmerman wrote:
> > > On Wed, 23 Jan 2019, Jani Nikula wrote:
> > >> On Wed, 23 Jan 2019, Greg KH wrote:
> > >> > On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> > >> >> Variables declared in a switch statement before any case statements
> > >> >> cannot be initialized, so move all instances out of the switches.
> > >> >> After this, future always-initialized stack variables will work
> > >> >> and not throw warnings like this:
> > >> >>
> > >> >> fs/fcntl.c: In function ‘send_sigio_to_task’:
> > >> >> fs/fcntl.c:738:13: warning: statement will never be executed
> > >> >> [-Wswitch-unreachable]
> > >> >>siginfo_t si;
> > >> >> ^~
> > >> >
> > >> > That's a pain, so this means we can't have any new variables in { }
> > >> > scope except for at the top of a function?
>
> Just in case this wasn't clear: no, it's just the switch statement
> before the first "case". I cannot imagine how bad it would be if we
> couldn't have block-scoped variables! Heh. :)
Sorry, it was not clear at first glance. So no more objection from me
for this change.
greg k-h
Re: MUSB interrupt storm on device removal
On Wed, Jan 23, 2019 at 02:12:38PM -0600, Bin Liu wrote: > > > > > Thanks for the info. > > > > > I will handle this case in musb driver. > > > > > > > > Why doesn't the same problem occur with other types of host controller? > > > > > > Not sure, I am on musb for most of the times. Maybe other HCD doesn't > > > giveback URBs with -EPROTO in such error case. > > > > ehci-hcd also uses -EPROTO. > > Is it possible to test the use case on ehci? > > - connect a multi-ports usb serial device to a hub; > - open multiple ports with cat command; > - remove the usb serial device from the hub; > - console lockup happens? No lockup happens at all for me :) Looks like this is a musb-only issue :( thanks, greg k-h
Re: MUSB interrupt storm on device removal
On Wed, Jan 23, 2019 at 11:05:40AM -0500, Alan Stern wrote: > On Wed, 23 Jan 2019, Bin Liu wrote: > > > On Wed, Jan 23, 2019 at 03:55:47PM +0100, Johan Hovold wrote: > > > On Wed, Jan 23, 2019 at 08:09:47AM -0600, Bin Liu wrote: > > > > On Wed, Jan 23, 2019 at 09:55:49AM +0100, Johan Hovold wrote: > > > > > On Wed, Jan 23, 2019 at 07:52:12AM +0100, Greg Kroah-Hartman wrote: > > > > > > > > > That's not what any other host controller returns when a device is > > > > > > removed, so either you are going to have to fix all USB drives for > > > > > > this > > > > > > issue, or you need to fix the musb driver to not send this error for > > > > > > when a device is removed (hint, do the latter...) > > > > > > > > > > Right, this needs to be handle at the HCD level. > > > > > > > > Any reason usb_serial_generic_read_bulk_callback() doesn't handle > > > > -EPROTO in the same way as -EPIPE? > > > > > > Since it is supposed to be intermittent unlike, for example, -ENOENT or > > > -EPIPE (the latter which the device driver can recover from if it cares > > > to implement clearing of halt). > > Wait a minute. Nothing says any of those errors is supposed to be > intermittent. As long as an error has a systematic cause (as opposed > to random noise, for example), it will recur as often as the cause > does. Right, I should have said drivers tend to treat it as always being intermittent. > At least when -EPROTO errors are caused by device disconnect, we know > that they will eventually go away when the upstream hub reports the > port disconnect event. But until then, an interrupt storm is certainly > possible. Indeed, and this isn't the first time we've had this discussion either I realised. In fact, I've been hit by this myself on BBB and musb when disconnecting a device connected through an external hub. At the time, the immediate causes that were making the completion handler take longer than usual (unaligned copy of a large buffer and a printk respectively) could be fixed. The problem went away, but of course not the underlying issue. Note that the problem I was seeing also went away both when switching to a different single-core SoC using ehci-omap, or when replacing the external hub. IIRC it wasn't the hub workqueue that was starved as a I first had thought, but the hub interrupt was never even received (or processed at least). Unfortunately, I never got around to investigating this further. > > > My point was that unless you fix this at the HCD level, you will need to > > > add complex recovery handling to every USB driver and completion handler > > > (~500 of those). But perhaps that is what it needed. > > > > okay, it probably make sense to handle the case in HCD because the > > number of HCD is much less. > > One possibility is to giveback URBs with certain errors (such as > -EPROTO) only at a frame boundary, or at 1-ms intervals. This feels > like a very artificial solution, though. > > > > I do see now that of all USB drivers we have two drivers that handles > > > -EPROTO by resubmitting after a delay, while a handful explicitly deals > > > with -EPROTO by simply stopping to resubmit (some probably bail out on > > > all errors, but the majority appear to resubmit on -EPROTO). > > Any driver which immediately retries an URB after getting -EPROTO or > -EILSEQ or -ETIME, and has no mechanism for backing off or limiting the > retries, is buggy. As far as I can see, that's all there is to it. I tend to agree with you on that, but adding complex back-off handling of intermittent errors to every USB driver and completion handler will be quite a bit of work. Unless the HCD drivers can assist, we'd at least want have some part of the implementation provided by shared code. Also note that simply starting to bail out on any error now would risk introducing regressions for setups where intermittent errors do occur. > > Thanks for the info. > > I will handle this case in musb driver. > > Why doesn't the same problem occur with other types of host controller? I think we should get to the bottom of that. BBB is single core which may be part of the reason why it's affected, but it's definitely related to the controller as well. Johan
Re: MUSB interrupt storm on device removal
On Wed, Jan 23, 2019 at 08:50:38PM +, Måns Rullgård wrote: > Bin Liu writes: > > >> > > Why doesn't the same problem occur with other types of host controller? > >> > > >> > Not sure, I am on musb for most of the times. Maybe other HCD doesn't > >> > giveback URBs with -EPROTO in such error case. > >> > >> ehci-hcd also uses -EPROTO. > > > > Is it possible to test the use case on ehci? > > > > - connect a multi-ports usb serial device to a hub; > > - open multiple ports with cat command; > > - remove the usb serial device from the hub; > > - console lockup happens? > > It doesn't seem to happen using ehci or even musb on Allwinner A20. > I have only seen the problem with musb on AM3358. The A20 being dual core may possible explain the difference. Johan
Re: [PATCH v5 3/4] Bluetooth: Allow driver specific cmd timeout handling
Hi Rajat,
> Add a hook to allow the BT driver to do device or command specific
> handling in case of timeouts. This is to be used by Intel driver to
> reset the device after certain number of timeouts.
>
> Signed-off-by: Rajat Jain
> ---
> v5: Drop the quirk, and rename the hook function to cmd_timeout()
> v4: same as v1
> v3: same as v1
> v2: same as v1
>
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 10 --
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h
> b/include/net/bluetooth/hci_core.h
> index e5ea633ea368..624d5f2b1f36 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -437,6 +437,7 @@ struct hci_dev {
> int (*post_init)(struct hci_dev *hdev);
> int (*set_diag)(struct hci_dev *hdev, bool enable);
> int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> + void (*cmd_timeout)(struct hci_dev *hdev, struct hci_command_hdr *cmd);
> };
>
> #define HCI_PHY_HANDLE(handle)(handle & 0xff)
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 7352fe85674b..c6917f57581a 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2568,16 +2568,22 @@ static void hci_cmd_timeout(struct work_struct *work)
> {
> struct hci_dev *hdev = container_of(work, struct hci_dev,
> cmd_timer.work);
> + struct hci_command_hdr *sent = NULL;
>
> if (hdev->sent_cmd) {
> - struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
> - u16 opcode = __le16_to_cpu(sent->opcode);
> + u16 opcode;
> +
> + sent = (void *) hdev->sent_cmd->data;
> + opcode = __le16_to_cpu(sent->opcode);
>
> bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
> } else {
> bt_dev_err(hdev, "command tx timeout");
> }
>
> + if (hdev->cmd_timeout)
> + hdev->cmd_timeout(hdev, sent);
> +
drop the sent parameter. You are not using it and if at some point in the
future you might want to use it, then we add it and fix up all users.
And frankly, I would move the hdev->cmd_timeout call into the hdev->sent_cmd
block since I do not even know if it makes sense to deal with the fallback case
where hdev->sent_cmd is not available. Unless you have that kind of error case,
but that is only possible if you have external injection of HCI commands.
Regards
Marcel
Re: [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip
Hi Rajat,
> If the platform provides it, use the reset gpio to reset the Intel BT
> chip, as part of cmd_timeout handling. This has been found helpful on
> Intel bluetooth controllers where the firmware gets stuck and the only
> way out is a hard reset pin provided by the platform.
>
> Signed-off-by: Rajat Jain
> ---
> v5: Rename the hook to cmd_timeout, and wait for 5 timeouts before
>resetting the device.
> v4: Use data->flags instead of clearing the quirk in btusb_hw_reset()
> v3: Better error handling for gpiod_get_optional()
> v2: Handle the EPROBE_DEFER case.
>
> drivers/bluetooth/btusb.c | 54 +++
> 1 file changed, 54 insertions(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 4761499db9ee..8949ea30a1bc 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -439,6 +440,7 @@ static const struct dmi_system_id
> btusb_needs_reset_resume_table[] = {
> #define BTUSB_BOOTING 9
> #define BTUSB_DIAG_RUNNING10
> #define BTUSB_OOB_WAKE_ENABLED11
> +#define BTUSB_HW_RESET_ACTIVE12
>
> struct btusb_data {
> struct hci_dev *hdev;
> @@ -476,6 +478,8 @@ struct btusb_data {
> struct usb_endpoint_descriptor *diag_tx_ep;
> struct usb_endpoint_descriptor *diag_rx_ep;
>
> + struct gpio_desc *reset_gpio;
> +
> __u8 cmdreq_type;
> __u8 cmdreq;
>
> @@ -489,8 +493,39 @@ struct btusb_data {
> int (*setup_on_usb)(struct hci_dev *hdev);
>
> int oob_wake_irq; /* irq for out-of-band wake-on-bt */
> + unsigned num_cmd_timeout;
Make this cmd_timeout_count or cmd_timeout_cnt.
> };
>
> +
> +static void btusb_intel_cmd_timeout(struct hci_dev *hdev,
> + struct hci_command_hdr *cmd)
> +{
> + struct btusb_data *data = hci_get_drvdata(hdev);
> + struct gpio_desc *reset_gpio = data->reset_gpio;
> +
> + bt_dev_err(hdev, "btusb num_cmd_timeout = %u", ++data->num_cmd_timeout);
I would prefer if you don’t do the increase in the bt_dev_err(). And you can
also scrap the “btusb “ prefix here since that is all present via bt_dev_err if
really needed at some point. Actually I question the whole bt_dev_err here in
the first place since the core will put our the error. You are just repeating
it here.
> +
> + if (data->num_cmd_timeout < 5)
> + return;
So I would propose to do just this:
if (++data->cmd_timeout_count < 5)
return;
> +
> + /*
> + * Toggle the hard reset line if the platform provides one. The reset
> + * is going to yank the device off the USB and then replug. So doing
> + * once is enough. The cleanup is handled correctly on the way out
> + * (standard USB disconnect), and the new device is detected cleanly
> + * and bound to the driver again like it should be.
> + */
> + if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
> + bt_dev_err(hdev, "last reset failed? Not resetting again");
> + return;
> + }
> +
> + bt_dev_err(hdev, "Initiating HW reset via gpio");
> + gpiod_set_value(reset_gpio, 1);
> + mdelay(100);
> + gpiod_set_value(reset_gpio, 0);
> +}
> +
> static inline void btusb_free_frags(struct btusb_data *data)
> {
> unsigned long flags;
> @@ -2915,6 +2950,7 @@ static int btusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> struct usb_endpoint_descriptor *ep_desc;
> + struct gpio_desc *reset_gpio;
> struct btusb_data *data;
> struct hci_dev *hdev;
> unsigned ifnum_base;
> @@ -3028,6 +3064,15 @@ static int btusb_probe(struct usb_interface *intf,
>
> SET_HCIDEV_DEV(hdev, &intf->dev);
>
> + reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
> + GPIOD_OUT_LOW);
Any reason to not use the devm_gpiod_get_optional() version here?
> + if (IS_ERR(reset_gpio)) {
> + err = PTR_ERR(reset_gpio);
> + goto out_free_dev;
> + } else if (reset_gpio) {
> + data->reset_gpio = reset_gpio;
> + }
> +
> hdev->open = btusb_open;
> hdev->close = btusb_close;
> hdev->flush = btusb_flush;
> @@ -3085,6 +3130,8 @@ static int btusb_probe(struct usb_interface *intf,
> set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
> set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> + if (data->reset_gpio)
> + hdev->cmd_timeout = btusb_intel_cmd_timeout;
> }
Always assign hdev->cmd_timeout = btusb_intel_cmd_timeout and put it after
btintel_set_bdaddr and before the quirks.
Move the if (data->reset_gpio) check into btusb_intel_cmd_timeout() functio
Re: [PATCH 6/8] power: reset: at91-reset: add support for sam9x60 SoC
Hi,
On Wed, Jan 16, 2019 at 10:57:42AM +0100, Nicolas Ferre wrote:
> Add support for additional reset causes and the proper compatibility
> string for sam9x60 SoC. The restart function is the same as the samx7.
>
> Signed-off-by: Nicolas Ferre
> ---
> drivers/power/reset/at91-reset.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/power/reset/at91-reset.c
> b/drivers/power/reset/at91-reset.c
> index f44a9ffcc2ab..44ca983a49a1 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -44,6 +44,9 @@ enum reset_type {
> RESET_TYPE_WATCHDOG = 2,
> RESET_TYPE_SOFTWARE = 3,
> RESET_TYPE_USER = 4,
> + RESET_TYPE_CPU_FAIL = 6,
> + RESET_TYPE_XTAL_FAIL= 7,
> + RESET_TYPE_ULP2 = 8,
what happened to 5? :)
> };
>
> static void __iomem *at91_ramc_base[2], *at91_rstc_base;
> @@ -164,6 +167,15 @@ static void __init at91_reset_status(struct
> platform_device *pdev)
> case RESET_TYPE_USER:
> reason = "user reset";
> break;
> + case RESET_TYPE_CPU_FAIL:
> + reason = "CPU clock failure detection";
> + break;
> + case RESET_TYPE_XTAL_FAIL:
> + reason = "32.768 kHz crystal failure detection";
> + break;
> + case RESET_TYPE_ULP2:
> + reason = "ULP2 reset";
> + break;
> default:
> reason = "unknown reset";
> break;
> @@ -183,6 +195,7 @@ static const struct of_device_id at91_reset_of_match[] = {
> { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
> { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
> { .compatible = "atmel,samx7-rstc", .data = samx7_restart },
> + { .compatible = "microchip,sam9x60-rstc", .data = samx7_restart },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, at91_reset_of_match);
Patch looks fine to me. But I will wait a bit with merging, so that
Alexandre or Ludovic have a chance to provide feedback.
-- Sebastian
signature.asc
Description: PGP signature
Re: [PATCH 6/8] power: reset: at91-reset: add support for sam9x60 SoC
Hi Sebastian,
On 23/01/2019 at 19:34, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Jan 16, 2019 at 10:57:42AM +0100, Nicolas Ferre wrote:
>> Add support for additional reset causes and the proper compatibility
>> string for sam9x60 SoC. The restart function is the same as the samx7.
>>
>> Signed-off-by: Nicolas Ferre
>> ---
>> drivers/power/reset/at91-reset.c | 13 +
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/power/reset/at91-reset.c
>> b/drivers/power/reset/at91-reset.c
>> index f44a9ffcc2ab..44ca983a49a1 100644
>> --- a/drivers/power/reset/at91-reset.c
>> +++ b/drivers/power/reset/at91-reset.c
>> @@ -44,6 +44,9 @@ enum reset_type {
>> RESET_TYPE_WATCHDOG = 2,
>> RESET_TYPE_SOFTWARE = 3,
>> RESET_TYPE_USER = 4,
>> +RESET_TYPE_CPU_FAIL = 6,
>> +RESET_TYPE_XTAL_FAIL= 7,
>> +RESET_TYPE_ULP2 = 8,
>
> what happened to 5? :)
That a good question ;-)
It's marked as "Reserved"... which opens up a whole new field of
speculation :-)
[..]
>> { .compatible = "atmel,samx7-rstc", .data = samx7_restart },
>> +{ .compatible = "microchip,sam9x60-rstc", .data = samx7_restart },
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, at91_reset_of_match);
>
> Patch looks fine to me. But I will wait a bit with merging, so that
> Alexandre or Ludovic have a chance to provide feedback.
What about merging this patch with the whole series through the at91
then arm-soc trees?
Best regards,
--
Nicolas Ferre
Re: MUSB interrupt storm on device removal
Johan Hovold writes: > On Wed, Jan 23, 2019 at 08:50:38PM +, Måns Rullgård wrote: >> Bin Liu writes: >> >> >> > > Why doesn't the same problem occur with other types of host >> >> > > controller? >> >> > >> >> > Not sure, I am on musb for most of the times. Maybe other HCD doesn't >> >> > giveback URBs with -EPROTO in such error case. >> >> >> >> ehci-hcd also uses -EPROTO. >> > >> > Is it possible to test the use case on ehci? >> > >> > - connect a multi-ports usb serial device to a hub; >> > - open multiple ports with cat command; >> > - remove the usb serial device from the hub; >> > - console lockup happens? >> >> It doesn't seem to happen using ehci or even musb on Allwinner A20. >> I have only seen the problem with musb on AM3358. > > The A20 being dual core may possible explain the difference. Booting the A20 with nosmp it still works correctly. -- Måns Rullgård
RE: [PATCH 1/3] treewide: Lift switch variables out of switches
On Wednesday, January 23, 2019 6:04 AM, Kees Cook wrote
>
> Variables declared in a switch statement before any case statements
> cannot be initialized, so move all instances out of the switches.
> After this, future always-initialized stack variables will work
> and not throw warnings like this:
>
> fs/fcntl.c: In function ‘send_sigio_to_task’:
> fs/fcntl.c:738:13: warning: statement will never be executed
> [-Wswitch-unreachable]
>siginfo_t si;
> ^~
>
> Signed-off-by: Kees Cook
Reviewed by: Edwin Zimmerman
> ---
> arch/x86/xen/enlighten_pv.c | 7 ---
> drivers/char/pcmcia/cm4000_cs.c | 2 +-
> drivers/char/ppdev.c | 20 ---
> drivers/gpu/drm/drm_edid.c| 4 ++--
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> drivers/net/ethernet/intel/e1000/e1000_main.c | 3 ++-
> drivers/tty/n_tty.c | 3 +--
> drivers/usb/gadget/udc/net2280.c | 5 ++---
> fs/fcntl.c| 3 ++-
> mm/shmem.c| 5 +++--
> net/core/skbuff.c | 4 ++--
> net/ipv6/ip6_gre.c| 4 ++--
> net/ipv6/ip6_tunnel.c | 4 ++--
> net/openvswitch/flow_netlink.c| 7 +++
> security/tomoyo/common.c | 3 ++-
> security/tomoyo/condition.c | 7 ---
> security/tomoyo/util.c| 4 ++--
> 18 files changed, 45 insertions(+), 46 deletions(-)
>
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index c54a493e139a..a79d4b548a08 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -907,14 +907,15 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err)
> static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high)
> {
> int ret;
> +#ifdef CONFIG_X86_64
> + unsigned which;
> + u64 base;
> +#endif
>
> ret = 0;
>
> switch (msr) {
> #ifdef CONFIG_X86_64
> - unsigned which;
> - u64 base;
> -
> case MSR_FS_BASE: which = SEGBASE_FS; goto set;
> case MSR_KERNEL_GS_BASE:which = SEGBASE_GS_USER; goto set;
> case MSR_GS_BASE: which = SEGBASE_GS_KERNEL; goto set;
> diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
> index 7a4eb86aedac..7211dc0e6f4f 100644
> --- a/drivers/char/pcmcia/cm4000_cs.c
> +++ b/drivers/char/pcmcia/cm4000_cs.c
> @@ -663,6 +663,7 @@ static void monitor_card(struct timer_list *t)
> {
> struct cm4000_dev *dev = from_timer(dev, t, timer);
> unsigned int iobase = dev->p_dev->resource[0]->start;
> + unsigned char flags0;
> unsigned short s;
> struct ptsreq ptsreq;
> int i, atrc;
> @@ -731,7 +732,6 @@ static void monitor_card(struct timer_list *t)
> }
>
> switch (dev->mstate) {
> - unsigned char flags0;
> case M_CARDOFF:
> DEBUGP(4, dev, "M_CARDOFF\n");
> flags0 = inb(REG_FLAGS0(iobase));
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index 1ae77b41050a..d77c97e4f996 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -359,14 +359,19 @@ static int pp_do_ioctl(struct file *file, unsigned int
> cmd, unsigned long arg)
> struct pp_struct *pp = file->private_data;
> struct parport *port;
> void __user *argp = (void __user *)arg;
> + struct ieee1284_info *info;
> + unsigned char reg;
> + unsigned char mask;
> + int mode;
> + s32 time32[2];
> + s64 time64[2];
> + struct timespec64 ts;
> + int ret;
>
> /* First handle the cases that don't take arguments. */
> switch (cmd) {
> case PPCLAIM:
> {
> - struct ieee1284_info *info;
> - int ret;
> -
> if (pp->flags & PP_CLAIMED) {
> dev_dbg(&pp->pdev->dev, "you've already got it!\n");
> return -EINVAL;
> @@ -517,15 +522,6 @@ static int pp_do_ioctl(struct file *file, unsigned int
> cmd, unsigned long arg)
>
> port = pp->pdev->port;
> switch (cmd) {
> - struct ieee1284_info *info;
> - unsigned char reg;
> - unsigned char mask;
> - int mode;
> - s32 time32[2];
> - s64 time64[2];
> - struct timespec64 ts;
> - int ret;
> -
> case PPRSTATUS:
> reg = parport_read_status(port);
> if (copy_to_user(argp, ®, sizeof(reg)))
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index b506e3622b08..8f93956c1628 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3942,12 +3942,12 @@ static voi
Re: [PATCH] net: usb: asix: ax88772_bind return error when hw_reset fail
On Thu, 2019-01-24 at 13:48 +0800, Zhang Run wrote:
> The ax88772_bind() should return error code immediately when the PHY
> was not reset properly through ax88772a_hw_reset().
> Otherwise, The asix_get_phyid() will block when get the PHY
> Identifier from the PHYSID1 MII registers through asix_mdio_read()
> due to the PHY isn't ready. Furthermore, it will produce a lot of
> error message cause system crash.As follows:
> asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to write
> reg index 0x: -71
> asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to send
> software reset: ffb9
> asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to write
> reg index 0x: -71
> asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to enable
> software MII access
> asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to read
> reg index 0x: -71
> asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to write
> reg index 0x: -71
> asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to enable
> software MII access
> asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to read
> reg index 0x: -71
> ...
>
> Signed-off-by: Zhang Run
> Reviewed-by: Yang Wei
Tested-by: Marcel Ziswiler
> ---
> drivers/net/usb/asix_devices.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/usb/asix_devices.c
> b/drivers/net/usb/asix_devices.c
> index b654f05..3d93993 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -739,8 +739,13 @@ static int ax88772_bind(struct usbnet *dev,
> struct usb_interface *intf)
> asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG, 0, 0, 1, &chipcode,
> 0);
> chipcode &= AX_CHIPCODE_MASK;
>
> - (chipcode == AX_AX88772_CHIPCODE) ? ax88772_hw_reset(dev, 0) :
> - ax88772a_hw_reset(dev, 0);
> + ret = (chipcode == AX_AX88772_CHIPCODE) ? ax88772_hw_reset(dev,
> 0) :
> + ax88772a_hw_reset(dev
> , 0);
> +
> + if (ret < 0) {
> + netdev_dbg(dev->net, "Failed to reset AX88772: %d\n",
> ret);
> + return ret;
> + }
>
> /* Read PHYID register *AFTER* the PHY was reset properly */
> phyid = asix_get_phyid(dev);
[PATCH] USB: serial: cp210x: support all gpios on CP2102N QFN28 package
The QFN28 package version of the CP2102N has three additional gpio pins.
Add support for these.
Signed-off-by: Mans Rullgard
---
drivers/usb/serial/cp210x.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index c0777a374a88..336a3c0f9f2c 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -1574,12 +1574,6 @@ static int cp2102n_gpioconf_init(struct usb_serial
*serial)
if (config_version != 0x01)
return -ENOTSUPP;
- /*
-* We only support 4 GPIOs even on the QFN28 package, because
-* config locations of GPIOs 4-6 determined using reverse
-* engineering revealed conflicting offsets with other
-* documented functions. So we'll just play it safe for now.
-*/
priv->gc.ngpio = 4;
/*
@@ -1594,6 +1588,19 @@ static int cp2102n_gpioconf_init(struct usb_serial
*serial)
/* 0 indicates GPIO mode, 1 is alternate function */
priv->gpio_altfunc = (gpio_ctrl >> 2) & 0x0f;
+ if (priv->partnum == CP210X_PARTNUM_CP2102N_QFN28) {
+ /*
+* For the QFN28 package, GPIO4-6 are controlled by
+* the low three bits of the mode/latch fields.
+* Contrary to the document linked above, the bits for
+* the SUSPEND pins are elsewhere. No alternate
+* function is available for these pins.
+*/
+ priv->gc.ngpio = 7;
+ gpio_latch |= (gpio_rst_latch & 7) << 4;
+ priv->gpio_pushpull |= (gpio_pushpull & 7) << 4;
+ }
+
/*
* The CP2102N does not strictly has input and output pin modes,
* it only knows open-drain and push-pull modes which is set at
--
2.20.1
Re: MUSB interrupt storm on device removal
On Thu, 24 Jan 2019, Johan Hovold wrote: > > At least when -EPROTO errors are caused by device disconnect, we know > > that they will eventually go away when the upstream hub reports the > > port disconnect event. But until then, an interrupt storm is certainly > > possible. > > Indeed, and this isn't the first time we've had this discussion either I > realised. > > In fact, I've been hit by this myself on BBB and musb when disconnecting > a device connected through an external hub. > > At the time, the immediate causes that were making the completion > handler take longer than usual (unaligned copy of a large buffer and a > printk respectively) could be fixed. The problem went away, but of > course not the underlying issue. > > Note that the problem I was seeing also went away both when switching to > a different single-core SoC using ehci-omap, or when replacing the > external hub. IIRC it wasn't the hub workqueue that was starved as a I > first had thought, but the hub interrupt was never even received (or > processed at least). > > Unfortunately, I never got around to investigating this further. I guess now we have some motivation to look into this more closely. :-) > > > > My point was that unless you fix this at the HCD level, you will need to > > > > add complex recovery handling to every USB driver and completion handler > > > > (~500 of those). But perhaps that is what it needed. > > > > > > okay, it probably make sense to handle the case in HCD because the > > > number of HCD is much less. > > > > One possibility is to giveback URBs with certain errors (such as > > -EPROTO) only at a frame boundary, or at 1-ms intervals. This feels > > like a very artificial solution, though. > > > > > > I do see now that of all USB drivers we have two drivers that handles > > > > -EPROTO by resubmitting after a delay, while a handful explicitly deals > > > > with -EPROTO by simply stopping to resubmit (some probably bail out on > > > > all errors, but the majority appear to resubmit on -EPROTO). > > > > Any driver which immediately retries an URB after getting -EPROTO or > > -EILSEQ or -ETIME, and has no mechanism for backing off or limiting the > > retries, is buggy. As far as I can see, that's all there is to it. > > I tend to agree with you on that, but adding complex back-off handling > of intermittent errors to every USB driver and completion handler will > be quite a bit of work. Unless the HCD drivers can assist, we'd at least > want have some part of the implementation provided by shared code. > > Also note that simply starting to bail out on any error now would risk > introducing regressions for setups where intermittent errors do occur. Agreed. > > Why doesn't the same problem occur with other types of host controller? > > I think we should get to the bottom of that. BBB is single core which > may be part of the reason why it's affected, but it's definitely related > to the controller as well. Perhaps it has something to do with the timing of the completion interrupts. I don't know anything about how musb works, though. Some low-level timing information would be good to see. Alan Stern
Re: MUSB interrupt storm on device removal
On Thu, Jan 24, 2019 at 12:56:33PM +, Måns Rullgård wrote: > Johan Hovold writes: > > > On Wed, Jan 23, 2019 at 08:50:38PM +, Måns Rullgård wrote: > >> Bin Liu writes: > >> > >> >> > > Why doesn't the same problem occur with other types of host > >> >> > > controller? > >> >> > > >> >> > Not sure, I am on musb for most of the times. Maybe other HCD doesn't > >> >> > giveback URBs with -EPROTO in such error case. > >> >> > >> >> ehci-hcd also uses -EPROTO. > >> > > >> > Is it possible to test the use case on ehci? > >> > > >> > - connect a multi-ports usb serial device to a hub; > >> > - open multiple ports with cat command; > >> > - remove the usb serial device from the hub; > >> > - console lockup happens? > >> > >> It doesn't seem to happen using ehci or even musb on Allwinner A20. > >> I have only seen the problem with musb on AM3358. > > > > The A20 being dual core may possible explain the difference. > > Booting the A20 with nosmp it still works correctly. Can you please debug it to see how the hub disconnect event got a chance to be processed? Regards, -Bin.
Re: MUSB interrupt storm on device removal
On Thu, Jan 24, 2019 at 10:22:26AM -0500, Alan Stern wrote: > On Thu, 24 Jan 2019, Johan Hovold wrote: > > > > At least when -EPROTO errors are caused by device disconnect, we know > > > that they will eventually go away when the upstream hub reports the > > > port disconnect event. But until then, an interrupt storm is certainly > > > possible. > > > > Indeed, and this isn't the first time we've had this discussion either I > > realised. > > > > In fact, I've been hit by this myself on BBB and musb when disconnecting > > a device connected through an external hub. > > > > At the time, the immediate causes that were making the completion > > handler take longer than usual (unaligned copy of a large buffer and a > > printk respectively) could be fixed. The problem went away, but of > > course not the underlying issue. > > > > Note that the problem I was seeing also went away both when switching to > > a different single-core SoC using ehci-omap, or when replacing the > > external hub. IIRC it wasn't the hub workqueue that was starved as a I > > first had thought, but the hub interrupt was never even received (or > > processed at least). > > > > Unfortunately, I never got around to investigating this further. > > I guess now we have some motivation to look into this more closely. :-) > > > > > > My point was that unless you fix this at the HCD level, you will need > > > > > to > > > > > add complex recovery handling to every USB driver and completion > > > > > handler > > > > > (~500 of those). But perhaps that is what it needed. > > > > > > > > okay, it probably make sense to handle the case in HCD because the > > > > number of HCD is much less. > > > > > > One possibility is to giveback URBs with certain errors (such as > > > -EPROTO) only at a frame boundary, or at 1-ms intervals. This feels > > > like a very artificial solution, though. > > > > > > > > I do see now that of all USB drivers we have two drivers that handles > > > > > -EPROTO by resubmitting after a delay, while a handful explicitly > > > > > deals > > > > > with -EPROTO by simply stopping to resubmit (some probably bail out on > > > > > all errors, but the majority appear to resubmit on -EPROTO). > > > > > > Any driver which immediately retries an URB after getting -EPROTO or > > > -EILSEQ or -ETIME, and has no mechanism for backing off or limiting the > > > retries, is buggy. As far as I can see, that's all there is to it. > > > > I tend to agree with you on that, but adding complex back-off handling > > of intermittent errors to every USB driver and completion handler will > > be quite a bit of work. Unless the HCD drivers can assist, we'd at least > > want have some part of the implementation provided by shared code. > > > > Also note that simply starting to bail out on any error now would risk > > introducing regressions for setups where intermittent errors do occur. > > Agreed. > > > > Why doesn't the same problem occur with other types of host controller? > > > > I think we should get to the bottom of that. BBB is single core which > > may be part of the reason why it's affected, but it's definitely related > > to the controller as well. > > Perhaps it has something to do with the timing of the completion > interrupts. I don't know anything about how musb works, though. Some > low-level timing information would be good to see. The musb controller driver itself doesn't have a isr BH, so when an ep interrupt happened, the isr directly processes the urb and called giveback. I tried to add HCD_BH to the musb hcd .flag, the issue still happens. Regards, -Bin.
Re: MUSB interrupt storm on device removal
On Thu, 24 Jan 2019, Bin Liu wrote: > > Perhaps it has something to do with the timing of the completion > > interrupts. I don't know anything about how musb works, though. Some > > low-level timing information would be good to see. > > The musb controller driver itself doesn't have a isr BH, so when an ep > interrupt happened, the isr directly processes the urb and called > giveback. I tried to add HCD_BH to the musb hcd .flag, the issue still > happens. I was thinking that maybe some controllers delay some types of completion interrupt until a frame boundary, while other controllers raise all completion interrupts immediately. Alan Stern
Re: kernel: xhci_hcd 0000:00:14.0: ERROR unknown event type 37 - Kernel 4.19.13
On 10.01.2019 00:11, Nathan Royce wrote: Wow, my system got wrecked (exaggeration) during this latest stretch... Pulseaudio was stretched to the limit and beyond and was forced to restart. Anything that was producing audio had to be restarted to get it back. This time was much like the first time and went from timestamp 573100.060927 (line 1) to 572506.604155 (line 11069), where 100% (literally) of it was that event 37 in the journal, no other kernel log entries except for the systemd-hostnamed audit before it all went down. And as usual, it was my USB TV tuner (tvheadend really) giving the Poll Timeout log entries. Those same uploaded trace files will be updated with the latest bugout. Hi. Finally had a chance to look at this. Sorry about the delay. Logs show event ring is full: 573047.104801: xhci_handle_event: EVENT: TRB status 'Event Ring Full Error' len 0 slot 0 ep 0 type 'Host Controller Event' flags e:C It's filled with 0 length short transfer events due to a the following loop: 1. Class driver asks for 58658 bytes from device (queues BULK IN URB) 2. short transfer event, xhci interrupts, saying we got less than 58658 bytes. We actually got 0 bytes. 3. return URB with zero bytes to class driver 4. Class driver immediately queues a new URB, asking for 58658 bytes (see step 1) Last 6ms before event ring is full this looped 255 times. one cycle of the loop in log: 573047.104748: xhci_handle_event: EVENT: TRB 00020b267770 status 'Short Packet' len 58658 slot 4 ep 7 type 'Transfer Event' flags e:C 573047.104749: xhci_handle_transfer: BULK: Buffer 1de2 length 58658 TD size 0 intr 0 type 'Normal' flags b:i:I:c:s:I:e:c 573047.104749: xhci_inc_deq: BULK 02f14758: enq 0x00020b267860(0x00020b267000) deq 0x00020b267780(0x00020b267000) segs 2 stream 0 free_trbs 495 bounce 512 cycle 0 573047.104752: xhci_urb_giveback: ep3in-bulk: urb 0bdcbe77 pipe 3225519232 slot 4 length 0/58658 sgs 0/0 stream 0 flags 00010200 573047.104758: xhci_urb_enqueue: ep3in-bulk: urb 0bdcbe77 pipe 3225519232 slot 4 length 0/58658 sgs 0/0 stream 0 flags 00010200 573047.104758: xhci_queue_trb: BULK: Buffer 1de2 length 58658 TD size 0 intr 0 type 'Normal' flags b:i:I:c:s:I:e:C 573047.104759: xhci_inc_enq: BULK 02f14758: enq 0x00020b267870(0x00020b267000) deq 0x00020b267780(0x00020b267000) segs 2 stream 0 free_trbs 494 bounce 512 cycle 0 573047.104759: xhci_inc_deq: EVENT 22f906c2: enq 0x00020b317000(0x00020b317000) deq 0x00020b3178d0(0x00020b317000) segs 1 stream 0 free_trbs 254 bounce 0 cycle 1 I'll continue digging into this -Mathias
Re: MUSB interrupt storm on device removal
On Thu, Jan 24, 2019 at 10:49:30AM -0500, Alan Stern wrote: > On Thu, 24 Jan 2019, Bin Liu wrote: > > > > Perhaps it has something to do with the timing of the completion > > > interrupts. I don't know anything about how musb works, though. Some > > > low-level timing information would be good to see. > > > > The musb controller driver itself doesn't have a isr BH, so when an ep > > interrupt happened, the isr directly processes the urb and called > > giveback. I tried to add HCD_BH to the musb hcd .flag, the issue still > > happens. > > I was thinking that maybe some controllers delay some types of > completion interrupt until a frame boundary, while other controllers > raise all completion interrupts immediately. It is interesting that A20 which uses musb controller too doesn't show the issue. We really have to compare the runtime on A20 and BeagleboneBlack to understand the problem. Regards, -Bin.
Re: MUSB interrupt storm on device removal
Bin Liu writes: > On Thu, Jan 24, 2019 at 12:56:33PM +, Måns Rullgård wrote: >> Johan Hovold writes: >> >> > On Wed, Jan 23, 2019 at 08:50:38PM +, Måns Rullgård wrote: >> >> Bin Liu writes: >> >> >> >> >> > > Why doesn't the same problem occur with other types of host >> >> >> > > controller? >> >> >> > >> >> >> > Not sure, I am on musb for most of the times. Maybe other HCD doesn't >> >> >> > giveback URBs with -EPROTO in such error case. >> >> >> >> >> >> ehci-hcd also uses -EPROTO. >> >> > >> >> > Is it possible to test the use case on ehci? >> >> > >> >> > - connect a multi-ports usb serial device to a hub; >> >> > - open multiple ports with cat command; >> >> > - remove the usb serial device from the hub; >> >> > - console lockup happens? >> >> >> >> It doesn't seem to happen using ehci or even musb on Allwinner A20. >> >> I have only seen the problem with musb on AM3358. >> > >> > The A20 being dual core may possible explain the difference. >> >> Booting the A20 with nosmp it still works correctly. > > Can you please debug it to see how the hub disconnect event got a chance > to be processed? Could you be a little more specific? I'm happy to run some tests, but I need to know what information you're looking for. -- Måns Rullgård
Re: [PATCH 6/8] power: reset: at91-reset: add support for sam9x60 SoC
Hi,
On Thu, Jan 24, 2019 at 10:34:50AM +, nicolas.fe...@microchip.com wrote:
> Hi Sebastian,
>
> On 23/01/2019 at 19:34, Sebastian Reichel wrote:
> > Hi,
> >
> > On Wed, Jan 16, 2019 at 10:57:42AM +0100, Nicolas Ferre wrote:
> >> Add support for additional reset causes and the proper compatibility
> >> string for sam9x60 SoC. The restart function is the same as the samx7.
> >>
> >> Signed-off-by: Nicolas Ferre
> >> ---
> >> drivers/power/reset/at91-reset.c | 13 +
> >> 1 file changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/power/reset/at91-reset.c
> >> b/drivers/power/reset/at91-reset.c
> >> index f44a9ffcc2ab..44ca983a49a1 100644
> >> --- a/drivers/power/reset/at91-reset.c
> >> +++ b/drivers/power/reset/at91-reset.c
> >> @@ -44,6 +44,9 @@ enum reset_type {
> >>RESET_TYPE_WATCHDOG = 2,
> >>RESET_TYPE_SOFTWARE = 3,
> >>RESET_TYPE_USER = 4,
> >> + RESET_TYPE_CPU_FAIL = 6,
> >> + RESET_TYPE_XTAL_FAIL= 7,
> >> + RESET_TYPE_ULP2 = 8,
> >
> > what happened to 5? :)
>
> That a good question ;-)
>
> It's marked as "Reserved"... which opens up a whole new field of
> speculation :-)
Ok :)
> [..]
>
> >>{ .compatible = "atmel,samx7-rstc", .data = samx7_restart },
> >> + { .compatible = "microchip,sam9x60-rstc", .data = samx7_restart },
> >>{ /* sentinel */ }
> >> };
> >> MODULE_DEVICE_TABLE(of, at91_reset_of_match);
> >
> > Patch looks fine to me. But I will wait a bit with merging, so that
> > Alexandre or Ludovic have a chance to provide feedback.
>
> What about merging this patch with the whole series through the at91
> then arm-soc trees?
It seems to be possible to merge this standalone, but merging
through at91/arm-soc is also fine with me.
Acked-by: Sebastian Reichel
-- Sebastian
signature.asc
Description: PGP signature
RE: [PATCH 1/7] usb: typec: ucsi: add get_fw_info function
Hi Greg
> -Original Message-
> From: Greg KH
> Sent: Thursday, January 17, 2019 11:06 PM
> To: Ajay Gupta
> Cc: heikki.kroge...@linux.intel.com; linux-usb@vger.kernel.org
> Subject: Re: [PATCH 1/7] usb: typec: ucsi: add get_fw_info function
>
> On Thu, Jan 17, 2019 at 05:09:03PM -0800, Ajay Gupta wrote:
> > Function is to get the details of ccg firmware and device version.
> >
> > Signed-off-by: Ajay Gupta
> > ---
> > drivers/usb/typec/ucsi/ucsi_ccg.c | 76
> > ++-
> > 1 file changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > index de8a43bdff68..4d35279ab853 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > @@ -17,15 +17,46 @@
> > #include
> > #include "ucsi.h"
> >
> > +struct ccg_dev_info {
> > + u8 fw_mode:2;
> > + u8 two_pd_ports:2;
> > + u8 row_size_256:2;
> > + u8:1; /* reserved */
> > + u8 hpi_v2_mode:1;
> > + u8 bl_mode:1;
> > + u8 cfgtbl_invalid:1;
> > + u8 fw1_invalid:1;
> > + u8 fw2_invalid:1;
> > + u8:4; /* reserved */
> > + u16 silicon_id;
> > + u16 bl_last_row;
> > +} __packed;
>
> Doesn't all of this break into tiny pieces when you read the memory into a
> big-
> endian system? Be very careful when using bitfields as a "raw"
> representation of a structure in memory.
I am planning to drop bit fields itself.
> > +
> > +struct version_format {
> > + u16 build;
> > + u8 patch;
> > + u8 min:4;
> > + u8 maj:4;
> > +};
>
> Same here.
>
> And not packed? That's dangerous :)
Same here.
>
>
> > +
> > +struct version_info {
> > + struct version_format base;
> > + struct version_format app;
> > +};
> > +
> > struct ucsi_ccg {
> > struct device *dev;
> > struct ucsi *ucsi;
> > struct ucsi_ppm ppm;
> > struct i2c_client *client;
> > + struct ccg_dev_info info;
> > };
> >
> > -#define CCGX_RAB_INTR_REG 0x06
> > -#define CCGX_RAB_UCSI_CONTROL 0x39
> > +#define CCGX_RAB_DEVICE_MODE 0x
> > +#define CCGX_RAB_INTR_REG 0x0006
> > +#define CCGX_RAB_READ_ALL_VER 0x0010
> > +#define CCGX_RAB_READ_FW2_VER 0x0020
> > +#define CCGX_RAB_UCSI_CONTROL 0x0039
> > #define CCGX_RAB_UCSI_CONTROL_STARTBIT(0)
> > #define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
> > #define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) &
> 0xff))
> > @@ -220,6 +251,41 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
> > return IRQ_HANDLED;
> > }
> >
> > +static int get_fw_info(struct ucsi_ccg *uc) {
> > + struct device *dev = uc->dev;
> > + struct version_info version[3];
> > + struct version_info *v;
> > + int err, i;
> > +
> > + err = ccg_read(uc, CCGX_RAB_READ_ALL_VER, (u8 *)(&version),
> > + sizeof(version));
> > + if (err < 0)
> > + return err;
> > +
> > + for (i = 1; i < ARRAY_SIZE(version); i++) {
> > + v = &version[i];
> > + dev_dbg(dev,
> > + "FW%d Version: %c%c v%x.%x%x, [Base
> %d.%d.%d.%d]\n",
> > + i, (v->app.build >> 8), (v->app.build & 0xFF),
> > + v->app.patch, v->app.maj, v->app.min,
> > + v->base.maj, v->base.min, v->base.patch,
> > + v->base.build);
> > + }
> > +
> > + err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
> > + sizeof(uc->info));
> > + if (err < 0)
> > + return err;
> > +
> > + dev_dbg(dev, "fw_mode: %d\n", uc->info.fw_mode);
> > + dev_dbg(dev, "fw1_invalid: %d\n", uc->info.fw1_invalid);
> > + dev_dbg(dev, "fw2_invalid: %d\n", uc->info.fw2_invalid);
> > + dev_dbg(dev, "silicon_id: 0x%04x\n", uc->info.silicon_id);
> > +
> > + return 0;
> > +}
> > +
> > static int ucsi_ccg_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > @@ -248,6 +314,12 @@ static int ucsi_ccg_probe(struct i2c_client *client,
> > return status;
> > }
> >
> > + status = get_fw_info(uc);
> > + if (status < 0) {
> > + dev_err(uc->dev, "get_fw_info failed - %d\n", status);
> > + return status;
>
> Are all devices required to have this information? if not, you just prevented
> them from working I think :(
The error is due to ccg_read() failure and in that case we want to stop.
Thanks
> nvpublic
> thanks,
>
> greg k-h
RE: [PATCH 1/7] usb: typec: ucsi: add get_fw_info function
Hi Heikki,
> > Function is to get the details of ccg firmware and device version.
> >
> > Signed-off-by: Ajay Gupta
> > ---
> > drivers/usb/typec/ucsi/ucsi_ccg.c | 76
> > ++-
> > 1 file changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > index de8a43bdff68..4d35279ab853 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > @@ -17,15 +17,46 @@
> > #include
> > #include "ucsi.h"
> >
> > +struct ccg_dev_info {
> > + u8 fw_mode:2;
> > + u8 two_pd_ports:2;
> > + u8 row_size_256:2;
> > + u8:1; /* reserved */
> > + u8 hpi_v2_mode:1;
> > + u8 bl_mode:1;
> > + u8 cfgtbl_invalid:1;
> > + u8 fw1_invalid:1;
> > + u8 fw2_invalid:1;
> > + u8:4; /* reserved */
> > + u16 silicon_id;
> > + u16 bl_last_row;
> > +} __packed;
> > +
> > +struct version_format {
> > + u16 build;
> > + u8 patch;
> > + u8 min:4;
> > + u8 maj:4;
> > +};
>
> Don't use bit fields. We shouldn't use them even in the core ucsi driver with
> the
> message data structures like we do.
So are you saying to remove bit fields from structure and instead use shift and
mask
on u8 variable to get bit fields?
> > +struct version_info {
> > + struct version_format base;
> > + struct version_format app;
> > +};
> > +
> > struct ucsi_ccg {
> > struct device *dev;
> > struct ucsi *ucsi;
> > struct ucsi_ppm ppm;
> > struct i2c_client *client;
> > + struct ccg_dev_info info;
> > };
> >
> > -#define CCGX_RAB_INTR_REG 0x06
> > -#define CCGX_RAB_UCSI_CONTROL 0x39
> > +#define CCGX_RAB_DEVICE_MODE 0x
> > +#define CCGX_RAB_INTR_REG 0x0006
> > +#define CCGX_RAB_READ_ALL_VER 0x0010
> > +#define CCGX_RAB_READ_FW2_VER 0x0020
> > +#define CCGX_RAB_UCSI_CONTROL 0x0039
> > #define CCGX_RAB_UCSI_CONTROL_STARTBIT(0)
> > #define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
> > #define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) &
> 0xff))
> > @@ -220,6 +251,41 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
> > return IRQ_HANDLED;
> > }
> >
> > +static int get_fw_info(struct ucsi_ccg *uc) {
> > + struct device *dev = uc->dev;
> > + struct version_info version[3];
> > + struct version_info *v;
> > + int err, i;
> > +
> > + err = ccg_read(uc, CCGX_RAB_READ_ALL_VER, (u8 *)(&version),
> > + sizeof(version));
> > + if (err < 0)
> > + return err;
> > +
> > + for (i = 1; i < ARRAY_SIZE(version); i++) {
> > + v = &version[i];
> > + dev_dbg(dev,
> > + "FW%d Version: %c%c v%x.%x%x, [Base
> %d.%d.%d.%d]\n",
> > + i, (v->app.build >> 8), (v->app.build & 0xFF),
> > + v->app.patch, v->app.maj, v->app.min,
> > + v->base.maj, v->base.min, v->base.patch,
> > + v->base.build);
>
> How about a sysfs file showing the fw version?
I can add that.
>
> > + }
> > +
> > + err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
> > + sizeof(uc->info));
> > + if (err < 0)
> > + return err;
> > +
> > + dev_dbg(dev, "fw_mode: %d\n", uc->info.fw_mode);
> > + dev_dbg(dev, "fw1_invalid: %d\n", uc->info.fw1_invalid);
> > + dev_dbg(dev, "fw2_invalid: %d\n", uc->info.fw2_invalid);
> > + dev_dbg(dev, "silicon_id: 0x%04x\n", uc->info.silicon_id);
>
> Are those prints really useful for anybody?
They are mostly informational purpose.
>
> If you need the values from that ccg_dev_info for debugging, then I think
> debugfs is the appropriate place to expose them.
Will remove them.
Thanks
> nvpublic
> > + return 0;
> > +}
> > +
> > static int ucsi_ccg_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > @@ -248,6 +314,12 @@ static int ucsi_ccg_probe(struct i2c_client *client,
> > return status;
> > }
> >
> > + status = get_fw_info(uc);
> > + if (status < 0) {
> > + dev_err(uc->dev, "get_fw_info failed - %d\n", status);
> > + return status;
> > + }
> > +
> > status = devm_request_threaded_irq(dev, client->irq, NULL,
> >ccg_irq_handler,
> >IRQF_ONESHOT |
> IRQF_TRIGGER_HIGH,
> > --
> > 2.17.1
>
> Br,
>
> --
> heikki
RE: [PATCH 5/7] usb: typec: ucsi: add fw update needed check
Hi Greg -off-by: Ajay Gupta > > --- > > drivers/usb/typec/ucsi/ucsi_ccg.c | 139 > > ++ > > 1 file changed, 139 insertions(+) > > > > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c > > b/drivers/usb/typec/ucsi/ucsi_ccg.c > > index 5f341934a5af..6069a9f60d1e 100644 > > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c > > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c > > @@ -9,6 +9,7 @@ > > */ > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -17,6 +18,8 @@ > > #include > > #include "ucsi.h" > > > > +static int secondary_fw_min_ver = 41; > > +module_param(secondary_fw_min_ver, int, 0660); > > Ick, never add new module parameters, this isn't the 1990's :) Ok 😊 > nvpublic
RE: [PATCH 5/7] usb: typec: ucsi: add fw update needed check
Hi Greg, > On Thu, Jan 17, 2019 at 05:12:38PM -0800, Ajay Gupta wrote: > > This will be needed to check if latest fw is already flashed. > > Very odd changelog text, please make this more detailed. Sure. > And your email threading broke here, did your email client change? There was some error from my email client and I had to resend few changes. It has been resolved now. Thanks > nvpublic > thanks, > > greg k-h
RE: [PATCH 5/7] usb: typec: ucsi: add fw update needed check
Hi Heikki,
> > Signed-off-by: Ajay Gupta
> > ---
> > drivers/usb/typec/ucsi/ucsi_ccg.c | 139
> > ++
> > 1 file changed, 139 insertions(+)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > index 5f341934a5af..6069a9f60d1e 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > @@ -9,6 +9,7 @@
> > */
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -17,6 +18,8 @@
> > #include
> > #include "ucsi.h"
> >
> > +static int secondary_fw_min_ver = 41;
> > +module_param(secondary_fw_min_ver, int, 0660);
> > #define CCGX_RAB_DEVICE_MODE 0x
> > #define CCGX_RAB_INTR_REG 0x0006
> > #define DEV_INT BIT(0)
> > @@ -68,6 +71,27 @@
> > #define FW2_METADATA_ROW0x1FE
> > #define FW_CFG_TABLE_SIG_SIZE 256
> >
> > +enum enum_fw_mode {
> > + BOOT, /* bootloader */
> > + FW1,/* FW partition-1 */
> > + FW2,/* FW partition-2 */
> > + FW_INVALID,
> > +};
> > +
> > +enum enum_flash_mode {
> > + SECONDARY_BL, /* update secondary using bootloader */
> > + SECONDARY, /* update secondary using primary */
> > + PRIMARY,/* update primary */
> > + FLASH_NOT_NEEDED, /* update not required */
> > + FLASH_INVALID,
> > +};
> > +
> > +static const char * const ccg_fw_names[] = {
> > + /* 0x00 */ "ccg_boot.cyacd",
> > + /* 0x01 */ "ccg_2.cyacd",
> > + /* 0x02 */ "ccg_1.cyacd",
> > +};
> > +
> > struct ccg_dev_info {
> > u8 fw_mode:2;
> > u8 two_pd_ports:2;
> > @@ -95,6 +119,20 @@ struct version_info {
> > struct version_format app;
> > };
> >
> > +struct fw_config_table {
> > + u32 identity;
> > + u16 table_size;
> > + u8 fwct_version;
> > + u8 is_key_change;
> > + u8 guid[16];
> > + struct version_format base;
> > + struct version_format app;
> > + u8 primary_fw_digest[32];
> > + u32 key_exp_length;
> > + u8 key_modulus[256];
> > + u8 key_exp[4];
> > +};
> > +
> > /* CCGx response codes */
> > enum ccg_resp_code {
> > CMD_NO_RESP = 0x00,
> > @@ -737,6 +775,107 @@ static int ccg_cmd_validate_fw(struct ucsi_ccg *uc,
> unsigned int fwid)
> > return 0;
> > }
> >
> > +static bool ccg_check_fw_version(struct ucsi_ccg *uc, const char *fw_name,
> > +struct version_format *app)
> > +{
> > + const struct firmware *fw = NULL;
> > + struct device *dev = uc->dev;
> > + struct fw_config_table fw_cfg;
> > + u32 cur_version, new_version;
> > + bool is_later = false;
> > +
> > + if (request_firmware(&fw, fw_name, dev) != 0) {
> > + dev_err(dev, "error: Failed to open cyacd file %s\n", fw_name);
> > + return false;
> > + }
> > +
> > + /*
> > +* check if signed fw
> > +* last part of fw image is fw cfg table and signature
> > +*/
> > + if (fw->size < sizeof(fw_cfg) + FW_CFG_TABLE_SIG_SIZE)
> > + goto not_signed_fw;
> > +
> > + memcpy((uint8_t *)&fw_cfg, fw->data + fw->size -
> > + sizeof(fw_cfg) - FW_CFG_TABLE_SIG_SIZE, sizeof(fw_cfg));
> > +
> > + if (fw_cfg.identity != ('F' | ('W' << 8) | ('C' << 16) | ('T' << 24))) {
> > + dev_info(dev, "not a signed image\n");
> > + goto not_signed_fw;
> > + }
> > +
> > + /* compare input version with FWCT version */
> > + cur_version = app->build | (app->patch << 16) |
> > + ((app->min | (app->maj << 4)) << 24);
> > +
> > + new_version = fw_cfg.app.build | (fw_cfg.app.patch << 16) |
> > + ((fw_cfg.app.min | (fw_cfg.app.maj << 4)) << 24);
> > +
> > + dev_dbg(dev, "compare current %08x and new version %08x\n",
> > + cur_version, new_version);
> > +
> > + if (new_version > cur_version) {
> > + dev_dbg(dev, "new firmware file version is later\n");
> > + is_later = true;
> > + } else {
> > + dev_dbg(dev, "new firmware file version is same or earlier\n");
> > + }
> > +
> > +not_signed_fw:
> > + release_firmware(fw);
> > + return is_later;
> > +}
> > +
> > +static int ccg_fw_update_needed(struct ucsi_ccg *uc,
> > + enum enum_flash_mode *mode)
> > +{
> > + struct device *dev = uc->dev;
> > + int err;
> > + struct version_info version[3];
> > +
> > + err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
> > + sizeof(uc->info));
> > + if (err) {
> > + dev_err(dev, "read device mode failed\n");
> > + return err;
> > + }
> > +
> > + err = ccg_read(uc, CCGX_RAB_READ_ALL_VER, (u8 *)version,
> > + sizeof(version));
> > + if (err) {
> > + dev_err(dev, "read device mode failed\n");
> > + return err;
> > + }
> > +
> > + dev_dbg(dev, "check if fw upgrade required %x %x %x %x %x %x %x
> %x\n",
> > + version[FW1].base.build, vers
RE: [PATCH 2/7] usb: typec: ucsi: add ccg command framework
Hi Greg, > On Thu, Jan 17, 2019 at 05:09:04PM -0800, Ajay Gupta wrote: > > Used to send command to ccg controller > > Writing changelog comments is hard, but please do more than just tiny snippets > like this. Explain _why_ this change is needed in detail please. > > Same for all of these patches, you are adding new functionality to the > kernel, I > think it deserves more than one sentance or less per commit, don't you? Sure. Will take care of it. Thanks > nvpublic > > thanks, > > greg k-h
RE: [PATCH 6/7] usb: typec: ucsi: add check for supported vendor
Hi Heikki, > From: Heikki Krogerus > Sent: Friday, January 18, 2019 7:30 AM > To: Ajay Gupta > Cc: linux-usb@vger.kernel.org > Subject: Re: [PATCH 6/7] usb: typec: ucsi: add check for supported vendor > > On Thu, Jan 17, 2019 at 05:13:02PM -0800, Ajay Gupta wrote: > > Added check to see the currently flashed or new firmware being flashed > > is from a supported vendor. > > Why separate patch for this? I thought its vendor specific changes; so added separately, looks like need to merge to patch 5/7. Thanks > nvpublic > thanks, > > -- > heikki
Re: [PATCH] usb: musb: Fix potential NULL dereference
By the way, why do we need to store the qh in urb->hcpriv?
qh can always be accessible through urb->ep->hcpriv
Wouldn't it be better to drop entire urb->hcpriv usage?
ср, 23 янв. 2019 г. в 20:52, Matwey V. Kornilov :
>
> We assign "urb->hcpriv = qh;" a few lines down. The valid qh for the urb is
> hep->hcpriv in this code path.
>
> Fixes: 714bc5ef3eda ("musb: potential use after free")
> Signed-off-by: Matwey V. Kornilov
> ---
> drivers/usb/musb/musb_host.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index c6118a962d37..6f267716768e 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -2336,7 +2336,7 @@ static int musb_urb_enqueue(
> * odd, rare, error prone, but legal.
> */
> kfree(qh);
> - qh = NULL;
> + qh = hep->hcpriv;
> ret = 0;
> } else
> ret = musb_schedule(musb, qh,
> --
> 2.16.4
>
--
With best regards,
Matwey V. Kornilov
RE: [PATCH 2/7] usb: typec: ucsi: add ccg command framework
Hi Heikki
> > Used to send command to ccg controller
> >
> > Signed-off-by: Ajay Gupta
> > ---
> > drivers/usb/typec/ucsi/ucsi_ccg.c | 252
> > --
> > 1 file changed, 243 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > index 4d35279ab853..dce9126b6a37 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > @@ -17,6 +17,29 @@
> > #include
> > #include "ucsi.h"
> >
> > +#define CCGX_RAB_DEVICE_MODE 0x
> > +#define CCGX_RAB_INTR_REG 0x0006
> > +#define DEV_INT BIT(0)
> > +#define PORT0_INT BIT(1)
> > +#define PORT1_INT BIT(2)
> > +#define UCSI_READ_INT BIT(7)
> > +#define CCGX_RAB_READ_ALL_VER 0x0010
> > +#define CCGX_RAB_READ_FW2_VER 0x0020
> > +#define CCGX_RAB_UCSI_CONTROL 0x0039
> > +#define CCGX_RAB_UCSI_CONTROL_STARTBIT(0)
> > +#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
> > +#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) &
> 0xff))
> > +#define DEV_REG_IDX
> CCGX_RAB_DEVICE_MODE
> > +#define CCGX_RAB_RESPONSE 0x007E
> > +#define ASYNC_EVENT BIT(7)
> > +
> > +/* CCGx events & async msg codes */
> > +#define RESET_COMPLETE 0x80
> > +#define EVENT_INDEXRESET_COMPLETE
> > +#define PORT_CONNECT_DET 0x84
> > +#define PORT_DISCONNECT_DET0x85
> > +#define ROLE_SWAP_COMPELETE0x87
> > +
> > struct ccg_dev_info {
> > u8 fw_mode:2;
> > u8 two_pd_ports:2;
> > @@ -44,23 +67,113 @@ struct version_info {
> > struct version_format app;
> > };
> >
> > +/* CCGx response codes */
> > +enum ccg_resp_code {
> > + CMD_NO_RESP = 0x00,
> > + CMD_SUCCESS = 0x02,
> > + FLASH_DATA_AVAILABLE= 0x03,
> > + CMD_INVALID = 0x05,
> > + FLASH_UPDATE_FAIL = 0x07,
> > + INVALID_FW = 0x08,
> > + INVALID_ARG = 0x09,
> > + CMD_NOT_SUPPORT = 0x0A,
> > + TRANSACTION_FAIL= 0x0C,
> > + PD_CMD_FAIL = 0x0D,
> > + UNDEF_ERROR = 0x0F,
> > + INVALID_RESP= 0x10,
> > +};
> > +
> > +static const char * const ccg_resp_strs[] = {
> > + /* 0x00 */ "No Response.",
> > + /* 0x01 */ "0x01",
> > + /* 0x02 */ "HPI Command Success.",
> > + /* 0x03 */ "Flash Data Available in data memory.",
> > + /* 0x04 */ "0x04",
> > + /* 0x05 */ "Invalid Command.",
> > + /* 0x06 */ "0x06",
> > + /* 0x07 */ "Flash write operation failed.",
> > + /* 0x08 */ "Firmware validity check failed.",
> > + /* 0x09 */ "Command failed due to invalid arguments.",
> > + /* 0x0A */ "Command not supported in the current mode.",
> > + /* 0x0B */ "0x0B",
> > + /* 0x0C */ "Transaction Failed. GOOD_CRC was not received.",
> > + /* 0x0D */ "PD Command Failed.",
> > + /* 0x0E */ "0x0E",
> > + /* 0x0F */ "Undefined Error",
> > +};
> > +
> > +static const char * const ccg_evt_strs[] = {
> > + /* 0x80 */ "Reset Complete.",
> > + /* 0x81 */ "Message queue overflow detected.",
> > + /* 0x82 */ "Overcurrent Detected",
> > + /* 0x83 */ "Overvoltage Detected",
> > + /* 0x84 */ "Type-C Port Connect Detected",
> > + /* 0x85 */ "Type-C Port Disconnect Detected",
> > + /* 0x86 */ "PD Contract Negotiation Complete",
> > + /* 0x87 */ "SWAP Complete",
> > + /* 0x88 */ "0x88",
> > + /* 0x89 */ "0x89",
> > + /* 0x8A */ "PS_RDY Message Received",
> > + /* 0x8B */ "GotoMin Message Received.",
> > + /* 0x8C */ "Accept Message Received",
> > + /* 0x8D */ "Reject Message Received",
> > + /* 0x8E */ "Wait Message Received",
> > + /* 0x8F */ "Hard Reset Received",
> > + /* 0x90 */ "VDM Received",
> > + /* 0x91 */ "Source Capabilities Message Received",
> > + /* 0x92 */ "Sink Capabilities Message Received",
> > + /* 0x93 */ "Display Port Alternate Mode entered",
> > + /* 0x94 */ "Display Port device connected at UFP_U",
> > + /* 0x95 */ "Display port device not connected at UFP_U",
> > + /* 0x96 */ "Display port SID not found in Discover SID process",
> > + /* 0x97 */ "Multiple SVIDs discovered along with DisplayPort SID",
> > + /* 0x98 */ "DP Functionality not supported by Cable",
> > + /* 0x99 */ "Display Port Configuration not supported by UFP",
> > + /* 0x9A */ "Hard Reset Sent to Port Partner",
> > + /* 0x9B */ "Soft Reset Sent to Port Partner",
> > + /* 0x9C */ "Cable Reset Sent to EMCA",
> > + /* 0x9D */ "Source Disabled State Entered",
> > + /* 0x9E */ "Sender Response Timer Timeout",
> > + /* 0x9F */ "No VDM Response Received",
> > + /* 0xA0 */ "Unexpected Voltage on Vbus",
> > + /* 0xA1 */ "Type-C Error Recovery",
> > + /* 0xA2 */ "0xA2",
> > + /* 0xA3 */ "
Re: [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip
On Thu, Jan 24, 2019 at 1:46 AM Marcel Holtmann wrote:
>
> Hi Rajat,
>
> > If the platform provides it, use the reset gpio to reset the Intel BT
> > chip, as part of cmd_timeout handling. This has been found helpful on
> > Intel bluetooth controllers where the firmware gets stuck and the only
> > way out is a hard reset pin provided by the platform.
> >
> > Signed-off-by: Rajat Jain
> > ---
> > v5: Rename the hook to cmd_timeout, and wait for 5 timeouts before
> >resetting the device.
> > v4: Use data->flags instead of clearing the quirk in btusb_hw_reset()
> > v3: Better error handling for gpiod_get_optional()
> > v2: Handle the EPROBE_DEFER case.
> >
> > drivers/bluetooth/btusb.c | 54 +++
> > 1 file changed, 54 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 4761499db9ee..8949ea30a1bc 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -29,6 +29,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include
> > @@ -439,6 +440,7 @@ static const struct dmi_system_id
> > btusb_needs_reset_resume_table[] = {
> > #define BTUSB_BOOTING 9
> > #define BTUSB_DIAG_RUNNING10
> > #define BTUSB_OOB_WAKE_ENABLED11
> > +#define BTUSB_HW_RESET_ACTIVE12
> >
> > struct btusb_data {
> > struct hci_dev *hdev;
> > @@ -476,6 +478,8 @@ struct btusb_data {
> > struct usb_endpoint_descriptor *diag_tx_ep;
> > struct usb_endpoint_descriptor *diag_rx_ep;
> >
> > + struct gpio_desc *reset_gpio;
> > +
> > __u8 cmdreq_type;
> > __u8 cmdreq;
> >
> > @@ -489,8 +493,39 @@ struct btusb_data {
> > int (*setup_on_usb)(struct hci_dev *hdev);
> >
> > int oob_wake_irq; /* irq for out-of-band wake-on-bt */
> > + unsigned num_cmd_timeout;
>
> Make this cmd_timeout_count or cmd_timeout_cnt.
Sure, will do.
>
> > };
> >
> > +
> > +static void btusb_intel_cmd_timeout(struct hci_dev *hdev,
> > + struct hci_command_hdr *cmd)
> > +{
> > + struct btusb_data *data = hci_get_drvdata(hdev);
> > + struct gpio_desc *reset_gpio = data->reset_gpio;
> > +
> > + bt_dev_err(hdev, "btusb num_cmd_timeout = %u",
> > ++data->num_cmd_timeout);
>
> I would prefer if you don’t do the increase in the bt_dev_err(). And you can
> also scrap the “btusb “ prefix here since that is all present via bt_dev_err
> if really needed at some point. Actually I question the whole bt_dev_err here
> in the first place since the core will put our the error. You are just
> repeating it here.
will do.
>
> > +
> > + if (data->num_cmd_timeout < 5)
> > + return;
>
> So I would propose to do just this:
>
> if (++data->cmd_timeout_count < 5)
> return;
will do.
>
> > +
> > + /*
> > + * Toggle the hard reset line if the platform provides one. The reset
> > + * is going to yank the device off the USB and then replug. So doing
> > + * once is enough. The cleanup is handled correctly on the way out
> > + * (standard USB disconnect), and the new device is detected cleanly
> > + * and bound to the driver again like it should be.
> > + */
> > + if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
> > + bt_dev_err(hdev, "last reset failed? Not resetting again");
> > + return;
> > + }
> > +
> > + bt_dev_err(hdev, "Initiating HW reset via gpio");
> > + gpiod_set_value(reset_gpio, 1);
> > + mdelay(100);
> > + gpiod_set_value(reset_gpio, 0);
> > +}
> > +
> > static inline void btusb_free_frags(struct btusb_data *data)
> > {
> > unsigned long flags;
> > @@ -2915,6 +2950,7 @@ static int btusb_probe(struct usb_interface *intf,
> > const struct usb_device_id *id)
> > {
> > struct usb_endpoint_descriptor *ep_desc;
> > + struct gpio_desc *reset_gpio;
> > struct btusb_data *data;
> > struct hci_dev *hdev;
> > unsigned ifnum_base;
> > @@ -3028,6 +3064,15 @@ static int btusb_probe(struct usb_interface *intf,
> >
> > SET_HCIDEV_DEV(hdev, &intf->dev);
> >
> > + reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
> > + GPIOD_OUT_LOW);
>
> Any reason to not use the devm_gpiod_get_optional() version here?
Yes, we're using the &data->udev->dev device (parent of the USB
interface) to fetch the gpio, so if we use the devm_* variant, the
gpio resource will not be freed immediately if the btusb_probe fails.
I'll add a comment in the code to make this more clear.
>
> > + if (IS_ERR(reset_gpio)) {
> > + err = PTR_ERR(reset_gpio);
> > + goto out_free_dev;
> > + } else if (reset_gpio) {
> > + data->reset_gpio = reset_gpio;
> > + }
> > +
> > hdev->open = btusb_open;
> > hdev->close = btusb_close;
> > hdev->flush = btusb_flush;
> >
Re: [PATCH v5 3/4] Bluetooth: Allow driver specific cmd timeout handling
Hi Marcel,
On Thu, Jan 24, 2019 at 1:36 AM Marcel Holtmann wrote:
>
> Hi Rajat,
>
> > Add a hook to allow the BT driver to do device or command specific
> > handling in case of timeouts. This is to be used by Intel driver to
> > reset the device after certain number of timeouts.
> >
> > Signed-off-by: Rajat Jain
> > ---
> > v5: Drop the quirk, and rename the hook function to cmd_timeout()
> > v4: same as v1
> > v3: same as v1
> > v2: same as v1
> >
> > include/net/bluetooth/hci_core.h | 1 +
> > net/bluetooth/hci_core.c | 10 --
> > 2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h
> > b/include/net/bluetooth/hci_core.h
> > index e5ea633ea368..624d5f2b1f36 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -437,6 +437,7 @@ struct hci_dev {
> > int (*post_init)(struct hci_dev *hdev);
> > int (*set_diag)(struct hci_dev *hdev, bool enable);
> > int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> > + void (*cmd_timeout)(struct hci_dev *hdev, struct hci_command_hdr
> > *cmd);
> > };
> >
> > #define HCI_PHY_HANDLE(handle)(handle & 0xff)
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 7352fe85674b..c6917f57581a 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -2568,16 +2568,22 @@ static void hci_cmd_timeout(struct work_struct
> > *work)
> > {
> > struct hci_dev *hdev = container_of(work, struct hci_dev,
> > cmd_timer.work);
> > + struct hci_command_hdr *sent = NULL;
> >
> > if (hdev->sent_cmd) {
> > - struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
> > - u16 opcode = __le16_to_cpu(sent->opcode);
> > + u16 opcode;
> > +
> > + sent = (void *) hdev->sent_cmd->data;
> > + opcode = __le16_to_cpu(sent->opcode);
> >
> > bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
> > } else {
> > bt_dev_err(hdev, "command tx timeout");
> > }
> >
> > + if (hdev->cmd_timeout)
> > + hdev->cmd_timeout(hdev, sent);
> > +
>
> drop the sent parameter. You are not using it and if at some point in the
> future you might want to use it, then we add it and fix up all users.
Sure, will do.
>
> And frankly, I would move the hdev->cmd_timeout call into the hdev->sent_cmd
> block since I do not even know if it makes sense to deal with the fallback
> case where hdev->sent_cmd is not available. Unless you have that kind of
> error case, but that is only possible if you have external injection of HCI
> commands.
Ummm ... my preference would be to keep it outside the block so that
the hook is always invoked in any timeout, to be consistent (whether
externally injected or not). Let me know if that is OK. I plan to send
out an iteration keeping it outside, but I'll be happy to move it in
if you feel strongly about it.
Thanks,
Rajat
>
> Regards
>
> Marcel
>
[PATCH v2] usbip: Fix vhci_urb_enqueue() URB null transfer buffer error path
Fix vhci_urb_enqueue() to print debug msg and return error instead of
failing with BUG_ON.
Signed-off-by: Shuah Khan
---
drivers/usb/usbip/vhci_hcd.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 1e592ec94ba4..f46ee1fefe02 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -702,8 +702,10 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct
urb *urb, gfp_t mem_flag
}
vdev = &vhci_hcd->vdev[portnum-1];
- /* patch to usb_sg_init() is in 2.5.60 */
- BUG_ON(!urb->transfer_buffer && urb->transfer_buffer_length);
+ if (!urb->transfer_buffer && urb->transfer_buffer_length) {
+ dev_dbg(dev, "Null URB transfer buffer\n");
+ return -EINVAL;
+ }
spin_lock_irqsave(&vhci->lock, flags);
--
2.17.1
[PATCH v6 1/4] usb: split code locating ACPI companion into port and device
From: Dmitry Torokhov
In preparation for handling embedded USB devices let's split
usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
usb_acpi_find_companion_for_port().
Signed-off-by: Dmitry Torokhov
Signed-off-by: Rajat Jain
Acked-by: Greg Kroah-Hartman
Tested-by: Sukumar Ghorai
---
v6: same as v4
v5: same as v4
v4: Add Acked-by and Tested-by in signatures.
v3: same as v1
v2: same as v1
drivers/usb/core/usb-acpi.c | 133 +++-
1 file changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e221861b3187..8ff73c83e8e8 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -139,12 +139,79 @@ static struct acpi_device *usb_acpi_find_port(struct
acpi_device *parent,
return acpi_find_child_device(parent, raw, false);
}
-static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+static struct acpi_device *
+usb_acpi_get_companion_for_port(struct usb_port *port_dev)
{
struct usb_device *udev;
struct acpi_device *adev;
acpi_handle *parent_handle;
+ int port1;
+
+ /* Get the struct usb_device point of port's hub */
+ udev = to_usb_device(port_dev->dev.parent->parent);
+
+ /*
+* The root hub ports' parent is the root hub. The non-root-hub
+* ports' parent is the parent hub port which the hub is
+* connected to.
+*/
+ if (!udev->parent) {
+ adev = ACPI_COMPANION(&udev->dev);
+ port1 = usb_hcd_find_raw_port_number(bus_to_hcd(udev->bus),
+port_dev->portnum);
+ } else {
+ parent_handle = usb_get_hub_port_acpi_handle(udev->parent,
+udev->portnum);
+ if (!parent_handle)
+ return NULL;
+
+ acpi_bus_get_device(parent_handle, &adev);
+ port1 = port_dev->portnum;
+ }
+
+ return usb_acpi_find_port(adev, port1);
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_port(struct usb_port *port_dev)
+{
+ struct acpi_device *adev;
+ struct acpi_pld_info *pld;
+ acpi_handle *handle;
+ acpi_status status;
+
+ adev = usb_acpi_get_companion_for_port(port_dev);
+ if (!adev)
+ return NULL;
+
+ handle = adev->handle;
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (!ACPI_FAILURE(status) && pld) {
+ port_dev->location = USB_ACPI_LOCATION_VALID
+ | pld->group_token << 8 | pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
+ ACPI_FREE(pld);
+ }
+ return adev;
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_device(struct usb_device *udev)
+{
+ struct acpi_device *adev;
+
+ if (!udev->parent)
+ return NULL;
+
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+}
+
+
+static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+{
/*
* In the ACPI DSDT table, only usb root hub and usb ports are
* acpi device nodes. The hierarchy like following.
@@ -158,66 +225,10 @@ static struct acpi_device *usb_acpi_find_companion(struct
device *dev)
* So all binding process is divided into two parts. binding
* root hub and usb ports.
*/
- if (is_usb_device(dev)) {
- udev = to_usb_device(dev);
- if (udev->parent)
- return NULL;
-
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(dev->parent);
- return acpi_find_child_device(adev, 0, false);
- } else if (is_usb_port(dev)) {
- struct usb_port *port_dev = to_usb_port(dev);
- int port1 = port_dev->portnum;
- struct acpi_pld_info *pld;
- acpi_handle *handle;
- acpi_status status;
-
- /* Get the struct usb_device point of port's hub */
- udev = to_usb_device(dev->parent->parent);
-
- /*
-* The root hub ports' parent is the root hub. The non-root-hub
-* ports' parent is the parent hub port which the hub is
-* connected to.
-*/
- if (!udev->parent) {
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- int raw;
-
- raw = usb_hcd_find_raw_port_number(hcd, port1);
-
- adev = usb_acpi_find_port(ACPI_COMPANION(&udev->dev),
- raw);
-
-
[PATCH v6 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip
If the platform provides it, use the reset gpio to reset the Intel BT
chip, as part of cmd_timeout handling. This has been found helpful on
Intel bluetooth controllers where the firmware gets stuck and the only
way out is a hard reset pin provided by the platform.
Signed-off-by: Rajat Jain
---
v6: Move the cmd_timeout() hook assignment with other hooks, move the
reset_gpio check in the timeout function.
v5: Rename the hook to cmd_timeout, and wait for 5 timeouts before
resetting the device.
v4: Use data->flags instead of clearing the quirk in btusb_hw_reset()
v3: Better error handling for gpiod_get_optional()
v2: Handle the EPROBE_DEFER case.
drivers/bluetooth/btusb.c | 54 +++
1 file changed, 54 insertions(+)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 4761499db9ee..5de0c2e59b97 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
#include
@@ -439,6 +440,7 @@ static const struct dmi_system_id
btusb_needs_reset_resume_table[] = {
#define BTUSB_BOOTING 9
#define BTUSB_DIAG_RUNNING 10
#define BTUSB_OOB_WAKE_ENABLED 11
+#define BTUSB_HW_RESET_ACTIVE 12
struct btusb_data {
struct hci_dev *hdev;
@@ -476,6 +478,8 @@ struct btusb_data {
struct usb_endpoint_descriptor *diag_tx_ep;
struct usb_endpoint_descriptor *diag_rx_ep;
+ struct gpio_desc *reset_gpio;
+
__u8 cmdreq_type;
__u8 cmdreq;
@@ -489,8 +493,41 @@ struct btusb_data {
int (*setup_on_usb)(struct hci_dev *hdev);
int oob_wake_irq; /* irq for out-of-band wake-on-bt */
+ unsigned cmd_timeout_cnt;
};
+
+static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
+{
+ struct btusb_data *data = hci_get_drvdata(hdev);
+ struct gpio_desc *reset_gpio = data->reset_gpio;
+
+ if (++data->cmd_timeout_cnt < 5)
+ return;
+
+ if (!reset_gpio) {
+ bt_dev_err(hdev, "No way to reset. Ignoring and continuing");
+ return;
+ }
+
+ /*
+* Toggle the hard reset line if the platform provides one. The reset
+* is going to yank the device off the USB and then replug. So doing
+* once is enough. The cleanup is handled correctly on the way out
+* (standard USB disconnect), and the new device is detected cleanly
+* and bound to the driver again like it should be.
+*/
+ if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
+ bt_dev_err(hdev, "last reset failed? Not resetting again");
+ return;
+ }
+
+ bt_dev_err(hdev, "Initiating HW reset via gpio");
+ gpiod_set_value(reset_gpio, 1);
+ mdelay(100);
+ gpiod_set_value(reset_gpio, 0);
+}
+
static inline void btusb_free_frags(struct btusb_data *data)
{
unsigned long flags;
@@ -2915,6 +2952,7 @@ static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct usb_endpoint_descriptor *ep_desc;
+ struct gpio_desc *reset_gpio;
struct btusb_data *data;
struct hci_dev *hdev;
unsigned ifnum_base;
@@ -3028,6 +3066,15 @@ static int btusb_probe(struct usb_interface *intf,
SET_HCIDEV_DEV(hdev, &intf->dev);
+ reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(reset_gpio)) {
+ err = PTR_ERR(reset_gpio);
+ goto out_free_dev;
+ } else if (reset_gpio) {
+ data->reset_gpio = reset_gpio;
+ }
+
hdev->open = btusb_open;
hdev->close = btusb_close;
hdev->flush = btusb_flush;
@@ -3082,6 +3129,7 @@ static int btusb_probe(struct usb_interface *intf,
hdev->shutdown = btusb_shutdown_intel;
hdev->set_diag = btintel_set_diag_mfg;
hdev->set_bdaddr = btintel_set_bdaddr;
+ hdev->cmd_timeout = btusb_intel_cmd_timeout;
set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
@@ -3094,6 +3142,7 @@ static int btusb_probe(struct usb_interface *intf,
hdev->hw_error = btintel_hw_error;
hdev->set_diag = btintel_set_diag;
hdev->set_bdaddr = btintel_set_bdaddr;
+ hdev->cmd_timeout = btusb_intel_cmd_timeout;
set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
@@ -3226,6 +3275,8 @@ static int btusb_probe(struct usb_interface *intf,
return 0;
out_free_dev:
+ if (data->reset_gpio)
[PATCH v6 3/4] Bluetooth: Allow driver specific cmd timeout handling
Add a hook to allow the BT driver to do device or command specific
handling in case of timeouts. This is to be used by Intel driver to
reset the device after certain number of timeouts.
Signed-off-by: Rajat Jain
---
v6: Dropped the "sent command" parameter from cmd_timeout()
v5: Drop the quirk, and rename the hook function to cmd_timeout()
v4: same as v1
v3: same as v1
v2: same as v1
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_core.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e5ea633ea368..094e61e07030 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -437,6 +437,7 @@ struct hci_dev {
int (*post_init)(struct hci_dev *hdev);
int (*set_diag)(struct hci_dev *hdev, bool enable);
int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
+ void (*cmd_timeout)(struct hci_dev *hdev);
};
#define HCI_PHY_HANDLE(handle) (handle & 0xff)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674b..75793265ba9e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2578,6 +2578,9 @@ static void hci_cmd_timeout(struct work_struct *work)
bt_dev_err(hdev, "command tx timeout");
}
+ if (hdev->cmd_timeout)
+ hdev->cmd_timeout(hdev);
+
atomic_set(&hdev->cmd_cnt, 1);
queue_work(hdev->workqueue, &hdev->cmd_work);
}
--
2.20.1.321.g9e740568ce-goog
[PATCH v6 2/4] usb: assign ACPI companions for embedded USB devices
From: Dmitry Torokhov
USB devices permanently connected to USB ports may be described in ACPI
tables and share ACPI devices with ports they are connected to. See [1]
for details.
This will allow us to describe sideband resources for devices, such as,
for example, hard reset line for BT USB controllers.
[1]
https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
Signed-off-by: Dmitry Torokhov
Signed-off-by: Rajat Jain (changed how we get the usb_port)
Acked-by: Greg Kroah-Hartman
Tested-by: Sukumar Ghorai
---
v6: same as v4
v5: same as v4
v4: Add Acked-by and Tested-by in signatures.
v3: same as v1
v2: same as v1
drivers/usb/core/usb-acpi.c | 44 +
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 8ff73c83e8e8..9043d7242d67 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -200,30 +200,56 @@ static struct acpi_device *
usb_acpi_find_companion_for_device(struct usb_device *udev)
{
struct acpi_device *adev;
+ struct usb_port *port_dev;
+ struct usb_hub *hub;
+
+ if (!udev->parent) {
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+ }
- if (!udev->parent)
+ hub = usb_hub_to_struct_hub(udev->parent);
+ if (!hub)
return NULL;
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(udev->dev.parent);
- return acpi_find_child_device(adev, 0, false);
+ /*
+* This is an embedded USB device connected to a port and such
+* devices share port's ACPI companion.
+*/
+ port_dev = hub->ports[udev->portnum - 1];
+ return usb_acpi_get_companion_for_port(port_dev);
}
-
static struct acpi_device *usb_acpi_find_companion(struct device *dev)
{
/*
-* In the ACPI DSDT table, only usb root hub and usb ports are
-* acpi device nodes. The hierarchy like following.
+* The USB hierarchy like following:
+*
* Device (EHC1)
* Device (HUBN)
* Device (PR01)
* Device (PR11)
* Device (PR12)
+* Device (FN12)
+* Device (FN13)
* Device (PR13)
* ...
-* So all binding process is divided into two parts. binding
-* root hub and usb ports.
+* where HUBN is root hub, and PRNN are USB ports and devices
+* connected to them, and FNNN are individualk functions for
+* connected composite USB devices. PRNN and FNNN may contain
+* _CRS and other methods describing sideband resources for
+* the connected device.
+*
+* On the kernel side both root hub and embedded USB devices are
+* represented as instances of usb_device structure, and ports
+* are represented as usb_port structures, so the whole process
+* is split into 2 parts: finding companions for devices and
+* finding companions for ports.
+*
+* Note that we do not handle individual functions of composite
+* devices yet, for that we would need to assign companions to
+* devices corresponding to USB interfaces.
*/
if (is_usb_device(dev))
return usb_acpi_find_companion_for_device(to_usb_device(dev));
--
2.20.1.321.g9e740568ce-goog
Re: [PATCH v4 1/6] dt-bindings: usb: musb: Add support for MediaTek musb controller
Hi Bin,
Thanks for your help.
Hi Rob,
I find that Samsung describes the usb-connector attribute in DTS, and
uses a private driver.
And try to write DTS as following:
usb-connector node:
musb_con: musb_connector{
compatible = "linux,extcon-usb-gpio","usb-b-connector";
lable = "micro-USB";
type = "micro";
id-gpio = <&pio 44 GPIO_ACTIVE_HIGH>
vbus-supply = <&usb_vbus>;
port {
usb_to_connector: endpoint {
remote-endpoint = <&connector_to_usb>;
};
};
};
usb node:
&usb2{
status = "okay";
port {
connector_to_usb: endpoint {
remote-endpoint = <&usb_to_connector>;
};
};
}
Can I describe usb-connector like this? Or can you give me some advices?
Regards,
Min.
On Tue, 2019-01-22 at 08:33 -0600, Bin Liu wrote:
> Hi Min,
>
> On Tue, Jan 22, 2019 at 05:36:13PM +0800, Min Guo wrote:
> > Hi Bin,
> >
> > Sorry to bother you again, I encounter a problem about the extcon
> > property.
> >
> > I don't find a common driver describing the usb-connector. Is
> > there any driver that I can refer to, specially the way to switch MUSB
> > controller between host and device mode?
> > If it needs to implement by myself, is it possible to emulate an
> > usb-connector driver by extcon-usb-gpio, and also use the notifier
> > mechanism or can you give me some advices?
>
> I am afraid I am unable to help you on this. I wasn't really pay
> attention when usb-connector was introduced and not sure how it can
> replace extcon. Now after read usb-connector.txt, it seems the binding
> only defines a/b/c-connector, but not ab-connector, and there is no
> enough information (at least for me) explaining how VBUS and ID fix into
> this usb-connector binding.
>
> Maybe Rob can provide some hint.
>
> Regards,
> -Bin.
[PATCH v4 5/5] usb :fsl: Change string format for errata property
From: Nikhil Badola Remove USB errata checking code from driver. Applicability of erratum is retrieved by reading corresponding property in device tree. This property is written during device tree fixup. Signed-off-by: Ramneek Mehresh Signed-off-by: Nikhil Badola Signed-off-by: Yinbo Zhu --- drivers/usb/host/fsl-mph-dr-of.c |7 ++- 1 files changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c index 762b976..ae8f60f 100644 --- a/drivers/usb/host/fsl-mph-dr-of.c +++ b/drivers/usb/host/fsl-mph-dr-of.c @@ -226,11 +226,8 @@ static int fsl_usb2_mph_dr_of_probe(struct platform_device *ofdev) of_property_read_bool(np, "fsl,usb_erratum-a005697"); pdata->has_fsl_erratum_a006918 = of_property_read_bool(np, "fsl,usb_erratum-a006918"); - - if (of_get_property(np, "fsl,usb_erratum_14", NULL)) - pdata->has_fsl_erratum_14 = 1; - else - pdata->has_fsl_erratum_14 = 0; + pdata->has_fsl_erratum_14 = + of_property_read_bool(np, "fsl,usb_erratum-14"); /* * Determine whether phy_clk_valid needs to be checked -- 1.7.1
[PATCH v4 3/5] usb: linux/fsl_device: Add platform member has_fsl_erratum_a006918
From: Yinbo Zhu
This patch is to add member has_fsl_erratum_a006918 in platform data
Signed-off-by: Yinbo Zhu
---
include/linux/fsl_devices.h |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index 5da56a6..4c613da 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -102,6 +102,7 @@ struct fsl_usb2_platform_data {
unsignedhas_fsl_erratum_14:1;
unsignedhas_fsl_erratum_a005275:1;
unsignedhas_fsl_erratum_a005697:1;
+ unsignedhas_fsl_erratum_a006918:1;
unsignedcheck_phy_clk_valid:1;
/* register save area for suspend/resume */
--
1.7.1
[PATCH v4 2/5] usb: phy: Workaround for USB erratum-A005728
From: Suresh Gupta
PHY_CLK_VALID bit for UTMI PHY in USBDR does not set even
if PHY is providing valid clock. Workaround for this
involves resetting of PHY and check PHY_CLK_VALID bit
multiple times. If PHY_CLK_VALID bit is still not set even
after 5 retries, it would be safe to deaclare that PHY
clock is not available.
This erratum is applicable for USBDR less then ver 2.4.
Signed-off-by: Suresh Gupta
Signed-off-by: Yinbo Zhu
---
Change in v4:
Incorrect indentation of the continuation line.
replace pr_err with dev_err.
drivers/usb/host/ehci-fsl.c | 38 +++---
drivers/usb/host/ehci-fsl.h |3 +++
2 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 38674b7..373a816 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -183,6 +183,17 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
return retval;
}
+static bool usb_phy_clk_valid(struct usb_hcd *hcd)
+{
+ void __iomem *non_ehci = hcd->regs;
+ bool ret = true;
+
+ if (!(ioread32be(non_ehci + FSL_SOC_USB_CTRL) & PHY_CLK_VALID))
+ ret = false;
+
+ return ret;
+}
+
static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
enum fsl_usb2_phy_modes phy_mode,
unsigned int port_offset)
@@ -226,6 +237,17 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
/* fall through */
case FSL_USB2_PHY_UTMI:
case FSL_USB2_PHY_UTMI_DUAL:
+ /* PHY_CLK_VALID bit is de-featured from all controller
+* versions below 2.4 and is to be checked only for
+* internal UTMI phy
+*/
+ if (pdata->controller_ver > FSL_USB_VER_2_4 &&
+ pdata->have_sysif_regs && !usb_phy_clk_valid(hcd)) {
+ dev_err(dev,
+ "%s: USB PHY clock invalid\n", dev_name(dev));
+ return -EINVAL;
+ }
+
if (pdata->have_sysif_regs && pdata->controller_ver) {
/* controller version 1.6 or above */
tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
@@ -249,17 +271,11 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
break;
}
- /*
-* check PHY_CLK_VALID to determine phy clock presence before writing
-* to portsc
-*/
- if (pdata->check_phy_clk_valid) {
- if (!(ioread32be(non_ehci + FSL_SOC_USB_CTRL) &
- PHY_CLK_VALID)) {
- dev_warn(hcd->self.controller,
-"USB PHY clock invalid\n");
- return -EINVAL;
- }
+ if (pdata->have_sysif_regs &&
+ pdata->controller_ver > FSL_USB_VER_1_6 &&
+ !usb_phy_clk_valid(hcd)) {
+ dev_warn(hcd->self.controller, "USB PHY clock invalid\n");
+ return -EINVAL;
}
ehci_writel(ehci, portsc, &ehci->regs->port_status[port_offset]);
diff --git a/drivers/usb/host/ehci-fsl.h b/drivers/usb/host/ehci-fsl.h
index cbc4220..9d18c6e 100644
--- a/drivers/usb/host/ehci-fsl.h
+++ b/drivers/usb/host/ehci-fsl.h
@@ -50,4 +50,7 @@
#define UTMI_PHY_EN (1<<9)
#define ULPI_PHY_CLK_SEL(1<<10)
#define PHY_CLK_VALID (1<<17)
+
+/* Retry count for checking UTMI PHY CLK validity */
+#define UTMI_PHY_CLK_VALID_CHK_RETRY 5
#endif /* _EHCI_FSL_H */
--
1.7.1
[PATCH v4 1/5] usb: fsl: Set USB_EN bit to select ULPI phy
From: Nikhil Badola Set USB_EN bit to select ULPI phy for USB controller version 2.5 Signed-off-by: Nikhil Badola Signed-off-by: Yinbo Zhu --- Change in v4: Incorrect indentation of the continuation line drivers/usb/host/ehci-fsl.c |6 ++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c index e3d0c1c..38674b7 100644 --- a/drivers/usb/host/ehci-fsl.c +++ b/drivers/usb/host/ehci-fsl.c @@ -122,6 +122,12 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev) tmp |= 0x4; iowrite32be(tmp, hcd->regs + FSL_SOC_USB_CTRL); } + + /* Set USB_EN bit to select ULPI phy for USB controller version 2.5 */ + if (pdata->controller_ver == FSL_USB_VER_2_5 && + pdata->phy_mode == FSL_USB2_PHY_ULPI) + iowrite32be(USB_CTRL_USB_EN, hcd->regs + FSL_SOC_USB_CTRL); + /* * Enable UTMI phy and program PTS field in UTMI mode before asserting * controller reset for USB Controller version 2.5 -- 1.7.1
[PATCH v4 4/5] usb: host: Stops USB controller init if PLL fails to lock
From: Ramneek Mehresh
USB erratum-A006918 workaround tries to start internal PHY inside
uboot (when PLL fails to lock). However, if the workaround also
fails, then USB initialization is also stopped inside Linux.
Erratum-A006918 workaround failure creates "fsl,erratum_a006918"
node in device-tree. Presence of this node in device-tree is
used to stop USB controller initialization in Linux
Signed-off-by: Ramneek Mehresh
Signed-off-by: Suresh Gupta
Signed-off-by: Yinbo Zhu
---
drivers/usb/host/ehci-fsl.c |5 +
drivers/usb/host/fsl-mph-dr-of.c |3 ++-
2 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 373a816..8b47277 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -236,6 +236,11 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
portsc |= PORT_PTS_PTW;
/* fall through */
case FSL_USB2_PHY_UTMI:
+ if (pdata->has_fsl_erratum_a006918) {
+ pr_warn("fsl-ehci: USB PHY clock invalid\n");
+ return -EINVAL;
+ }
+
case FSL_USB2_PHY_UTMI_DUAL:
/* PHY_CLK_VALID bit is de-featured from all controller
* versions below 2.4 and is to be checked only for
diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c
index 4f8b8a0..762b976 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -224,13 +224,14 @@ static int fsl_usb2_mph_dr_of_probe(struct
platform_device *ofdev)
of_property_read_bool(np, "fsl,usb-erratum-a005275");
pdata->has_fsl_erratum_a005697 =
of_property_read_bool(np, "fsl,usb_erratum-a005697");
+ pdata->has_fsl_erratum_a006918 =
+ of_property_read_bool(np, "fsl,usb_erratum-a006918");
if (of_get_property(np, "fsl,usb_erratum_14", NULL))
pdata->has_fsl_erratum_14 = 1;
else
pdata->has_fsl_erratum_14 = 0;
-
/*
* Determine whether phy_clk_valid needs to be checked
* by reading property in device tree
--
1.7.1
[PATCH] usb: chipidea: imx: Remove unused include
Remove unused #include in drivers/usb/chipidea/ci_hdrc_imx.c Signed-off-by: Alexander Shiyan --- drivers/usb/chipidea/ci_hdrc_imx.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c index 09b37c0d075d..a1c0e9cc73a6 100644 --- a/drivers/usb/chipidea/ci_hdrc_imx.c +++ b/drivers/usb/chipidea/ci_hdrc_imx.c @@ -7,7 +7,6 @@ #include #include -#include #include #include #include -- 2.13.0
RE: [PATCH] usb: chipidea: imx: Remove unused include
> Cc: Peter Chen ; Greg Kroah-Hartman > ; Alexander Shiyan > Subject: [PATCH] usb: chipidea: imx: Remove unused include > > Remove unused #include in drivers/usb/chipidea/ci_hdrc_imx.c > > Signed-off-by: Alexander Shiyan > --- > drivers/usb/chipidea/ci_hdrc_imx.c | 1 - > 1 file changed, 1 deletion(-) > Thanks, Jun Li just posted a patch for it some days before. > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c > b/drivers/usb/chipidea/ci_hdrc_imx.c > index 09b37c0d075d..a1c0e9cc73a6 100644 > --- a/drivers/usb/chipidea/ci_hdrc_imx.c > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c > @@ -7,7 +7,6 @@ > > #include > #include > -#include > #include > #include > #include > -- > 2.13.0
Re: [PATCH] net: usb: asix: ax88772_bind return error when hw_reset fail
From: Zhang Run Date: Thu, 24 Jan 2019 13:48:49 +0800 > The ax88772_bind() should return error code immediately when the PHY > was not reset properly through ax88772a_hw_reset(). > Otherwise, The asix_get_phyid() will block when get the PHY > Identifier from the PHYSID1 MII registers through asix_mdio_read() > due to the PHY isn't ready. Furthermore, it will produce a lot of > error message cause system crash.As follows: > asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to write > reg index 0x: -71 > asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to send > software reset: ffb9 > asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to write > reg index 0x: -71 > asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to enable > software MII access > asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to read > reg index 0x: -71 > asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to write > reg index 0x: -71 > asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to enable > software MII access > asix 1-1:1.0 (unnamed net_device) (uninitialized): Failed to read > reg index 0x: -71 > ... > > Signed-off-by: Zhang Run > Reviewed-by: Yang Wei Applied.
Re: [PATCH v6 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip
Hi Rajat, > If the platform provides it, use the reset gpio to reset the Intel BT > chip, as part of cmd_timeout handling. This has been found helpful on > Intel bluetooth controllers where the firmware gets stuck and the only > way out is a hard reset pin provided by the platform. > > Signed-off-by: Rajat Jain > --- > v6: Move the cmd_timeout() hook assignment with other hooks, move the >reset_gpio check in the timeout function. > v5: Rename the hook to cmd_timeout, and wait for 5 timeouts before >resetting the device. > v4: Use data->flags instead of clearing the quirk in btusb_hw_reset() > v3: Better error handling for gpiod_get_optional() > v2: Handle the EPROBE_DEFER case. > > drivers/bluetooth/btusb.c | 54 +++ > 1 file changed, 54 insertions(+) patch has been applied to bluetooth-next tree. Regards Marcel
Re: [PATCH v6 2/4] usb: assign ACPI companions for embedded USB devices
Hi Rajat, > USB devices permanently connected to USB ports may be described in ACPI > tables and share ACPI devices with ports they are connected to. See [1] > for details. > > This will allow us to describe sideband resources for devices, such as, > for example, hard reset line for BT USB controllers. > > [1] > https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices > > Signed-off-by: Dmitry Torokhov > Signed-off-by: Rajat Jain (changed how we get the > usb_port) > Acked-by: Greg Kroah-Hartman > Tested-by: Sukumar Ghorai > --- > v6: same as v4 > v5: same as v4 > v4: Add Acked-by and Tested-by in signatures. > v3: same as v1 > v2: same as v1 > > drivers/usb/core/usb-acpi.c | 44 + > 1 file changed, 35 insertions(+), 9 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel
Re: [PATCH v6 1/4] usb: split code locating ACPI companion into port and device
Hi Rajat, > In preparation for handling embedded USB devices let's split > usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and > usb_acpi_find_companion_for_port(). > > Signed-off-by: Dmitry Torokhov > Signed-off-by: Rajat Jain > Acked-by: Greg Kroah-Hartman > Tested-by: Sukumar Ghorai > --- > v6: same as v4 > v5: same as v4 > v4: Add Acked-by and Tested-by in signatures. > v3: same as v1 > v2: same as v1 > > drivers/usb/core/usb-acpi.c | 133 +++- > 1 file changed, 72 insertions(+), 61 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel
Re: [PATCH v6 3/4] Bluetooth: Allow driver specific cmd timeout handling
Hi Rajat, > Add a hook to allow the BT driver to do device or command specific > handling in case of timeouts. This is to be used by Intel driver to > reset the device after certain number of timeouts. > > Signed-off-by: Rajat Jain > --- > v6: Dropped the "sent command" parameter from cmd_timeout() > v5: Drop the quirk, and rename the hook function to cmd_timeout() > v4: same as v1 > v3: same as v1 > v2: same as v1 > > include/net/bluetooth/hci_core.h | 1 + > net/bluetooth/hci_core.c | 3 +++ > 2 files changed, 4 insertions(+) patch has been applied to bluetooth-next tree. Regards Marcel
Re: usb: dwc2: usb data transmitted to incorrect usb endpoint
Hi Maynard,
On 1/24/2019 12:13 AM, Maynard Cabiente wrote:
> Hi Minas,
>
> On Wednesday, January 23, 2019 3:58 AM Minas Harutyunyan
> wrote:
>> But before it, could you please add some debug print in
>> dwc2_hsotg_suspend() function. I suspect that host side autosuspend mass
>> storage device(s) this is why dwc2 driver complete request with ERROR:
>> -108. Based on debug log:
>>
>> [ 368.964378] dwc2 ffb4.usb: dwc2_hsotg_ep_queue: [req ce60a380]
>> ep1 out: 512@ce188000, noi=0, zero=0, snok=0 (new CBW for ep1)
>> [ 368.967886] configfs-gadget gadget: End Point Request ERROR: -108
>> <-- ERROR DUE TO USB DATA TRANSMITTED TO INCORRECT USB EP
>>
>> I see >3ms inactivity which can be reason to suspend bus.
>> If my assumption is correct then you can disable autosuspend feature of
>> connected devices.
>>
>
> There is actually a debug log in dwc2_hsotg_suspend function():
>
> if (hsotg->driver) {
> int ep;
> dev_info(hsotg->dev, "suspending usb gadget %s\n",
>hsotg->driver->driver.name);
> ...
>
> However, I searched in my logs on all the runs I did that reproduced
> the issue and that message is not present on all of them. So, suspend
> is not happening in this case.
>
>> I don't know about email attachments size limits. Actually, you can
>> truncate (save as required parts) whole USB trace.
>>
>
> Posting below the USB trace (exported text file) where the problem lies.
>
> Regards,
> Maynard
>
Besides, mentioned by you timestamp <23.918 564 900>, where BULK IN data
messed between EP1IN and EP2IN, I found in USB trace one more invalid
traffic packets sequence. See in trace at timestamp <24.169 347 217>: on
same EP1 to CBW (Mode Sense) command 31 bytes, device reply as IN data
128 bytes and first 31 bytes are SAME from CBW. That mean buffer pointer
set incorrectly EP1IN pointer set to EP1OUT pointer. In case of DDMA
buffer pointer is descriptor pointer.
Difference between this 2 cases:
1. When EP different (your case) SCSI Tag's also different this is why
class driver issue reset.
2. When EP same then (my case) SCSI Tag's are same, just wrong data from
class driver point of view, and later class driver again send Mode Sense
command which completed OK and no reset initiated.
I not found yet where from come this bug.
Meanwhile, could you please:
1. Test same scenario in BDMA mode
2. Provide me full debug messages log for DDMA around when the issue seen.
It's not core bug it's dwc2 bug.
Thanks,
Minas
