Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi Mathias,
I have some comments for the implementation of
xhci_handle_command_timeout() as well.
On 12/20/2016 11:13 PM, Mathias Nyman wrote:
> On 20.12.2016 09:30, Baolin Wang wrote:
> ...
>
> Alright, I gathered all current work related to xhci races and timeouts
> and put them into a branch:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
> timeout_race_fixes
>
> Its based on 4.9
> It includes a few other patches just to avoid conflicts and make my life
> easier
>
> Interesting patches are:
>
> ee4eb91 xhci: remove unnecessary check for pending timer
> 0cba67d xhci: detect stop endpoint race using pending timer instead of
> counter.
> 4f2535f xhci: Handle command completion and timeout race
> b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort command
> 529a5a0 usb: xhci: fix possible wild pointer
> 4766555 xhci: Fix race related to abort operation
> de834a3 xhci: Use delayed_work instead of timer for command timeout
> 69973b8 Linux 4.9
>
> The fixes for command queue races will go to usb-linus and stable, the
> reworks for stop ep watchdog timer will go to usb-next.
>
> Still completely untested, (well it compiles)
>
> Felipe gave instructions how to modify dwc3 driver to timeout on address
> devicecommands to test these, I'll try to set that up.
>
> All additional testing is welcome, especially if you can trigger timeouts
> and races
>
> -Mathias
>
>
I post the code below and add my comments in line.
1276 void xhci_handle_command_timeout(struct work_struct *work)
1277 {
1278 struct xhci_hcd *xhci;
1279 int ret;
1280 unsigned long flags;
1281 u64 hw_ring_state;
1282
1283 xhci = container_of(to_delayed_work(work), struct xhci_hcd,
cmd_timer);
1284
1285 spin_lock_irqsave(&xhci->lock, flags);
1286
1287 /*
1288 * If timeout work is pending, or current_cmd is NULL, it means we
1289 * raced with command completion. Command is handled so just
return.
1290 */
1291 if (!xhci->current_cmd || delayed_work_pending(&xhci->cmd_timer)) {
1292 spin_unlock_irqrestore(&xhci->lock, flags);
1293 return;
1294 }
1295 /* mark this command to be cancelled */
1296 xhci->current_cmd->status = COMP_CMD_ABORT;
1297
1298 /* Make sure command ring is running before aborting it */
1299 hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
1300 if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
1301 (hw_ring_state & CMD_RING_RUNNING)) {
1302 /* Prevent new doorbell, and start command abort */
1303 xhci->cmd_ring_state = CMD_RING_STATE_ABORTED;
1304 spin_unlock_irqrestore(&xhci->lock, flags);
1305 xhci_dbg(xhci, "Command timeout\n");
1306 ret = xhci_abort_cmd_ring(xhci);
1307 if (unlikely(ret == -ESHUTDOWN)) {
1308 xhci_err(xhci, "Abort command ring failed\n");
1309 xhci_cleanup_command_queue(xhci);
1310 usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
1311 xhci_dbg(xhci, "xHCI host controller is dead.\n");
1312 }
1313 return;
1314 }
1315
1316 /* host removed. Bail out */
1317 if (xhci->xhc_state & XHCI_STATE_REMOVING) {
1318 spin_unlock_irqrestore(&xhci->lock, flags);
1319 xhci_dbg(xhci, "host removed, ring start fail?\n");
1320 xhci_cleanup_command_queue(xhci);
1321 return;
1322 }
I think this part of code should be moved up to line 1295.
1323
1324 /* command timeout on stopped ring, ring can't be aborted */
1325 xhci_dbg(xhci, "Command timeout on stopped ring\n");
1326 xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd);
1327 spin_unlock_irqrestore(&xhci->lock, flags);
This part of code is tricky. I have no idea about in which case should this
code be executed? Anyway, we shouldn't call xhci_handle_stopped_cmd_ring()
here, right?
1328 return;
1329 }
Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi,
On 12/21/2016 08:57 PM, Mathias Nyman wrote:
> On 21.12.2016 08:57, Lu Baolu wrote:
>> Hi Mathias,
>>
>> I have some comments for the implementation of
>> xhci_handle_command_timeout() as well.
>>
>> On 12/20/2016 11:13 PM, Mathias Nyman wrote:
>>> On 20.12.2016 09:30, Baolin Wang wrote:
>>> ...
>>>
>>> Alright, I gathered all current work related to xhci races and timeouts
>>> and put them into a branch:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
>>> timeout_race_fixes
>>>
>>> Its based on 4.9
>>> It includes a few other patches just to avoid conflicts and make my life
>>> easier
>>>
>>> Interesting patches are:
>>>
>>> ee4eb91 xhci: remove unnecessary check for pending timer
>>> 0cba67d xhci: detect stop endpoint race using pending timer instead of
>>> counter.
>>> 4f2535f xhci: Handle command completion and timeout race
>>> b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort
>>> command
>>> 529a5a0 usb: xhci: fix possible wild pointer
>>> 4766555 xhci: Fix race related to abort operation
>>> de834a3 xhci: Use delayed_work instead of timer for command timeout
>>> 69973b8 Linux 4.9
>>>
>>> The fixes for command queue races will go to usb-linus and stable, the
>>> reworks for stop ep watchdog timer will go to usb-next.
>>>
>>> Still completely untested, (well it compiles)
>>>
>>> Felipe gave instructions how to modify dwc3 driver to timeout on address
>>> devicecommands to test these, I'll try to set that up.
>>>
>>> All additional testing is welcome, especially if you can trigger timeouts
>>> and races
>>>
>>> -Mathias
>>>
>>>
>>
>> I post the code below and add my comments in line.
>>
>> 1276 void xhci_handle_command_timeout(struct work_struct *work)
>> 1277 {
>> 1278 struct xhci_hcd *xhci;
>> 1279 int ret;
>> 1280 unsigned long flags;
>> 1281 u64 hw_ring_state;
>> 1282
>> 1283 xhci = container_of(to_delayed_work(work), struct xhci_hcd,
>> cmd_timer);
>> 1284
>> 1285 spin_lock_irqsave(&xhci->lock, flags);
>> 1286
>> 1287 /*
>> 1288 * If timeout work is pending, or current_cmd is NULL, it means
>> we
>> 1289 * raced with command completion. Command is handled so just
>> return.
>> 1290 */
>> 1291 if (!xhci->current_cmd ||
>> delayed_work_pending(&xhci->cmd_timer)) {
>> 1292 spin_unlock_irqrestore(&xhci->lock, flags);
>> 1293 return;
>> 1294 }
>> 1295 /* mark this command to be cancelled */
>> 1296 xhci->current_cmd->status = COMP_CMD_ABORT;
>> 1297
>> 1298 /* Make sure command ring is running before aborting it */
>> 1299 hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
>> 1300 if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
>> 1301 (hw_ring_state & CMD_RING_RUNNING)) {
>> 1302 /* Prevent new doorbell, and start command abort */
>> 1303 xhci->cmd_ring_state = CMD_RING_STATE_ABORTED;
>> 1304 spin_unlock_irqrestore(&xhci->lock, flags);
>> 1305 xhci_dbg(xhci, "Command timeout\n");
>> 1306 ret = xhci_abort_cmd_ring(xhci);
>> 1307 if (unlikely(ret == -ESHUTDOWN)) {
>> 1308 xhci_err(xhci, "Abort command ring failed\n");
>> 1309 xhci_cleanup_command_queue(xhci);
>> 1310 usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
>> 1311 xhci_dbg(xhci, "xHCI host controller is
>> dead.\n");
>> 1312 }
>> 1313 return;
>> 1314 }
>> 1315
>> 1316 /* host removed. Bail out */
>> 1317 if (xhci->xhc_state & XHCI_STATE_REMOVING) {
>> 1318 spin_unlock_irqrestore(&xhci->lock, flags);
>> 1319 xhci_dbg(xhci, "host removed, ring start fail?\n");
>> 1320 xhci_cleanup_command_queue(xhci);
>> 1321 return;
>> 1322 }
>>
>> I think this part of code should be moved up to line 1295.
>
> The XHCI_STATE
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi,
On 12/21/2016 08:48 PM, Mathias Nyman wrote:
> On 21.12.2016 08:17, Lu Baolu wrote:
>> Hi Mathias,
>>
>> I have some comments for the implementation of xhci_abort_cmd_ring() below.
>>
>> On 12/20/2016 11:13 PM, Mathias Nyman wrote:
>>> On 20.12.2016 09:30, Baolin Wang wrote:
>>> ...
>>>
>>> Alright, I gathered all current work related to xhci races and timeouts
>>> and put them into a branch:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
>>> timeout_race_fixes
>>>
>>> Its based on 4.9
>>> It includes a few other patches just to avoid conflicts and make my life
>>> easier
>>>
>>> Interesting patches are:
>>>
>>> ee4eb91 xhci: remove unnecessary check for pending timer
>>> 0cba67d xhci: detect stop endpoint race using pending timer instead of
>>> counter.
>>> 4f2535f xhci: Handle command completion and timeout race
>>> b9d00d7 usb: host: xhci: Fix possible wild pointer when handling abort
>>> command
>>> 529a5a0 usb: xhci: fix possible wild pointer
>>> 4766555 xhci: Fix race related to abort operation
>>> de834a3 xhci: Use delayed_work instead of timer for command timeout
>>> 69973b8 Linux 4.9
>>>
>>> The fixes for command queue races will go to usb-linus and stable, the
>>> reworks for stop ep watchdog timer will go to usb-next.
>>>
>>> Still completely untested, (well it compiles)
>>>
>>> Felipe gave instructions how to modify dwc3 driver to timeout on address
>>> devicecommands to test these, I'll try to set that up.
>>>
>>> All additional testing is welcome, especially if you can trigger timeouts
>>> and races
>>>
>>> -Mathias
>>>
>>>
>>
>> Below is the latest code. I put my comments in line.
>>
>> 322 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
>> 323 {
>> 324 u64 temp_64;
>> 325 int ret;
>> 326
>> 327 xhci_dbg(xhci, "Abort command ring\n");
>> 328
>> 329 reinit_completion(&xhci->cmd_ring_stop_completion);
>> 330
>> 331 temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
>> 332 xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
>> 333 &xhci->op_regs->cmd_ring);
>>
>> We should hold xhci->lock when we are modifying xhci registers
>> at runtime.
>>
>
> Makes sense, but we need to unlock it before sleeping or waiting for
> completion.
> I need to look into that in more detail.
>
> But this was an issue already before these changes.
>
>> The retry of setting CMD_RING_ABORT is not necessary according to
>> previous discussion. We have cleaned code for second try in
>> xhci_handle_command_timeout(). Need to clean up here as well.
>>
>
> Yes it can be cleaned up as well, but the two cases are a bit different.
> The cleaned up one was about command ring not starting again after it was
> stopped.
>
> This second try is a workaround for what we thought was the command ring
> failing
> to stop in the first place, but is most likely due to the race that OGAWA
> Hirofumi
> fixed. It races if the stop command ring interrupt happens between writing
> the abort
> bit and polling for the ring stopped bit. The interrupt hander may start the
> command
> ring again, and we would believe we failed to stop it in the first place.
>
> This race could probably be fixed by just extending the lock (and preventing
> interrupts) to cover both writing the abort bit and polling for the command
> ring
> running bit, as you pointed out here previously.
>
> But then again I really like OGAWA Hiroumi's solution that separates the
> command ring stopping from aborting commands and restarting the ring.
>
> The current way of always restarting the command ring as a response to
> a stop command ring command really limits its usage.
>
> So, with this in mind most reasonable would be to
> 1. fix the lock to cover abort+CRR check, and send it to usb-linus +stable
> 2. rebase OGAWA Hirofumi's changes on top of that, and send to usb-linus only
> 3. remove unnecessary second abort try as a separate patch, send to usb-next
> 4. remove polling for the Command ring running (CRR), waiting for completion
>is enough, if completion times out then we can check CRR. for usb-next
>I'll fix the typos these patches would introduce. Fixing old typos can be
> done as separate
> patches later.
This is exactly the same as what I am thinking of. I will submit the patches
later.
Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi, On 12/21/2016 11:18 PM, OGAWA Hirofumi wrote: > Mathias Nyman writes: > >>> We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what >>> is for taking lock for register though, I guess it should be enough just >>> lock around of read=>write of ->cmd_ring if need lock. >> After your patch it should be enough to have the lock only while >> reading and writing the cmd_ring register. >> >> If we want a locking fix that applies more easily to older stable >> releases before your change then the lock needs to cover set >> CMD_RING_STATE_ABORT, read cmd_reg, write cmd_reg and busiloop >> checking CRR bit. Otherwise the stop cmd ring interrupt handler may >> restart the ring just before we start checing CRR. The stop cmd ring >> interrupt will set the CMD_RING_STATE_ABORTED to >> CMD_RING_STATE_RUNNING so ring will really restart in the interrupt >> handler. > Just for record (no chance to make patch I myself for now, sorry), while > checking locking slightly, I noticed unrelated missing locking. > > xhci_cleanup_command_queue() > > We are calling it without locking, but we need to lock for accessing list. Yeah. I can make the patch. Best regards, Lu Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] phy: Add new Intel Cherrytrail USB OTG phy driver
Hi,
On 12/22/2016 07:47 PM, Hans de Goede wrote:
> +static int intel_cht_usb_phy_probe(struct platform_device *pdev)
> +{
> + struct intel_cht_usb_phy *phy;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + resource_size_t size;
> + int i, ret;
> +
> + phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
> + if (!phy)
> + return -ENOMEM;
> +
> + phy->dev = dev;
> + phy->mode = PHY_MODE_USB_OTG;
> + INIT_WORK(&phy->work, intel_cht_usb_phy_work);
> + platform_set_drvdata(pdev, phy);
> +
> + phy->id_extcon = extcon_get_extcon_dev(USB_HOST_EXTCON_DEV_NAME);
> + if (phy->id_extcon == NULL) {
> + dev_dbg(dev, "id_extcon is not ready, probe deferred\n");
> + return -EPROBE_DEFER;
> + }
> +
> + phy->vbus_extcon = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME);
> + if (phy->vbus_extcon == NULL) {
> + dev_dbg(dev, "vbus_extcon is not ready, probe deferred\n");
> + return -EPROBE_DEFER;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + size = (res->end + 1) - res->start;
"res" is used without check.
> + phy->base = devm_ioremap_nocache(dev, res->start, size);
This resource is part of xHCI MMIO. xHCI driver has already declared
to use the whole memory space. I am afraid you are not able to do this
due to resource conflict.
Best regards,
Lu Baolu
> + if (IS_ERR(phy->base)) {
> + ret = PTR_ERR(phy->base);
> + dev_err(dev, "can't iomap registers: %d\n", ret);
> + return ret;
> + }
> +
> + phy->phy = devm_phy_create(dev, NULL, &intel_cht_usb_phy_ops);
> + if (IS_ERR(phy->phy)) {
> + ret = PTR_ERR(phy->phy);
> + dev_err(dev, "can't create PHY: %d\n", ret);
> + return ret;
> + }
> + phy_set_drvdata(phy->phy, phy);
> +
> + /* Register for id notification */
> + phy->id_nb.notifier_call = intel_cht_usb_phy_id_cable_evt;
> + ret = devm_extcon_register_notifier(dev, phy->id_extcon,
> + EXTCON_USB_HOST, &phy->id_nb);
> + if (ret) {
> + dev_err(dev, "can't register id extcon notifier: %d\n", ret);
> + return ret;
> + }
> +
> + /* Register for vbus notification */
> + phy->vbus_nb[0].notifier_call = intel_cht_usb_phy_vbus_cable0_evt;
> + phy->vbus_nb[1].notifier_call = intel_cht_usb_phy_vbus_cable1_evt;
> + phy->vbus_nb[2].notifier_call = intel_cht_usb_phy_vbus_cable2_evt;
> + for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
> + ret = devm_extcon_register_notifier(dev, phy->vbus_extcon,
> + vbus_cable_ids[i], &phy->vbus_nb[i]);
> + if (ret) {
> + dev_err(dev, "can't register extcon notifier for %u:
> %d\n",
> + vbus_cable_ids[i], ret);
> + return ret;
> + }
> + }
> +
> + /* Get and process initial cable states */
> + schedule_work(&phy->work);
> +
> + device_create_file(dev, &dev_attr_mode);
> +
> + return phy_create_lookup(phy->phy, "dwc3.0", "usb3-phy");
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] Intel cherrytrail xhci extended cap phy/mux support
Hi, On 12/22/2016 09:18 PM, Felipe Balbi wrote: > Hi, > > Hans de Goede writes: >>>> Here are 2 patches which can and should be merged separately, but >>>> which do belong together, as together they add support for the >>>> usb-phy / mux bits found in the Intel Cherrytrail xhci vendor defined >>>> extended capabilities registers. >>>> >>>> I did not want to hide an entire phy driver inside the xhci code, >>>> nor did I want to add an extcon dependency to the xhci code, so >>>> I've chosen to simply make the xhci code register a platform childdev >>>> with an IOMEM resource with the Intel Cherrytrail xhci vendor defined >>>> extended capabilities registers when these are found. This keeps the >>>> xhci driver clean and allows writing a separate phy driver, this is >>>> the first patch, which should be merged through the USB tree. >>>> >>>> The second patch adds the actual phy driver, one thing which stands >>>> out is that it completely depends on extcon devices to get idpin and >>>> vbus state as that is done by other chips on cherrytrail devices, >>>> atm the extcon devices are hardcoded to axp288_extcon for the vbus >>>> detection and the ACPI INT3496:00 device for the idpin detection. >>>> >>>> On some boards a different pmic then the axp288 may be used, so in >>>> the future we may end up with an array with extcon device names to >>>> try until we've found one. >>>> >>>> The phy driver also adds a mode sysfs attribute, this is useful for >>>> testing, as well with a so called charging oth hub, where the idpin >>>> will indicate device/charge mode, but we can also use usb-devices >>>> plugged into the hub by switching the data lines to host mode. >>> Baolu has worked on this for months but it turned out that several folks >>> said 'no' to his patchset. You're not really dealing with a PHY, it's >>> just a portmux which can generate some UTMI messages to make sure dwc3 >>> is happy. >> Right, I opted for a phy driver because that is the closest I could >> get to something resembling what this thing does. >> >>> The end of the story about this, at least for broxton, was that mux >>> control was moved to ASL. I'm not sure why folks didn't update CHT (or >>> other devices') BIOS though. >> I don't think expecting things to get "fixed" by firmware updates, esp. >> firmware updates adding whole new functionality is going to happen, that >> just is not realistic (esp. with cheap devices like these). And even if >> it were fixed in firmware users are really bad add updating there firmware. > True that. If you can convince all folks involved that a PHY driver or a > portmux framework is acceptable, then more power for you :-) > > Frankly, though, this should always have been handled at ASL level > because port role is really decided at cable connection time > (well... not 100% true). > >>> Baolu, any comments to this series? >>> >>> Personally, I'm unwilling to take this (or the other 2-patch dwc3 series >>> adding IDA) because there will only be a single user. >> Only a single user ? You mean only a single SoC will use this I guess ? > yeah > >> But there are many many devices with this SoC and lots of people are >> trying to run "plain" Linux on these. > don't get me wrong. I'd really like to have Up Board running mainline > with no extra hacks necessary ;-) > >> I can see how you don't want this as a phy driver though, I would >> be happy to drop the phy-binding bits (they're not really used >> anyways) and drop this under drivers/misc adding myself to MAINTAINERS >> for it, would that be an acceptable solution ? > For me? sure :-) Let's get Greg and Oliver to agree though. > >> About the "other 2-patch dwc3 series", I'm fine with you not taking >> the IDA patch, but the first patch is unrelated to IDA, it is a pure >> bug-fix and really should be upstreamed. > right, that's true. > >> Matthias, assuming Felipe is ok with putting this in drivers/misc >> (minus the phy bindings), are you ok with the xhci blurb which >> registers the platform-device for the "misc" driver to bind to? > This is the one thing Greg was against. The creation of the "fake" > platform-device from XHCI. Greg was unhappy with my portmux patch set due to "faking a platform-device from a PCI device". We need to avoid this. Best regards, Lu Baolu > I mean, turning it into a real PCI device > will require firmware update, so we might as well update the firmware to > move MUX handling to ASL altogether ;-) > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] Intel cherrytrail xhci extended cap phy/mux support
Hi, On 12/23/2016 06:10 AM, Hans de Goede wrote: > Hi, > > On 22-12-16 17:28, Mathias Nyman wrote: >> On 22.12.2016 14:45, Hans de Goede wrote: >>> Hi, >>> >>> On 22-12-16 13:11, Felipe Balbi wrote: >>>> >>>> Hi, >>>> >>>> Hans de Goede writes: >>>>> Hi All, >>>>> >>>>> Here are 2 patches which can and should be merged separately, but >>>>> which do belong together, as together they add support for the >>>>> usb-phy / mux bits found in the Intel Cherrytrail xhci vendor defined >>>>> extended capabilities registers. >>>>> >>>>> I did not want to hide an entire phy driver inside the xhci code, >>>>> nor did I want to add an extcon dependency to the xhci code, so >>>>> I've chosen to simply make the xhci code register a platform childdev >>>>> with an IOMEM resource with the Intel Cherrytrail xhci vendor defined >>>>> extended capabilities registers when these are found. This keeps the >>>>> xhci driver clean and allows writing a separate phy driver, this is >>>>> the first patch, which should be merged through the USB tree. >>>>> >>>>> The second patch adds the actual phy driver, one thing which stands >>>>> out is that it completely depends on extcon devices to get idpin and >>>>> vbus state as that is done by other chips on cherrytrail devices, >>>>> atm the extcon devices are hardcoded to axp288_extcon for the vbus >>>>> detection and the ACPI INT3496:00 device for the idpin detection. >>>>> >>>>> On some boards a different pmic then the axp288 may be used, so in >>>>> the future we may end up with an array with extcon device names to >>>>> try until we've found one. >>>>> >>>>> The phy driver also adds a mode sysfs attribute, this is useful for >>>>> testing, as well with a so called charging oth hub, where the idpin >>>>> will indicate device/charge mode, but we can also use usb-devices >>>>> plugged into the hub by switching the data lines to host mode. >>>> >>>> Baolu has worked on this for months but it turned out that several folks >>>> said 'no' to his patchset. You're not really dealing with a PHY, it's >>>> just a portmux which can generate some UTMI messages to make sure dwc3 >>>> is happy. >>> >>> Right, I opted for a phy driver because that is the closest I could >>> get to something resembling what this thing does. >>> >>>> The end of the story about this, at least for broxton, was that mux >>>> control was moved to ASL. I'm not sure why folks didn't update CHT (or >>>> other devices') BIOS though. >>> >>> I don't think expecting things to get "fixed" by firmware updates, esp. >>> firmware updates adding whole new functionality is going to happen, that >>> just is not realistic (esp. with cheap devices like these). And even if >>> it were fixed in firmware users are really bad add updating there firmware. >>> >>>> Baolu, any comments to this series? >>>> >>>> Personally, I'm unwilling to take this (or the other 2-patch dwc3 series >>>> adding IDA) because there will only be a single user. >>> >>> Only a single user ? You mean only a single SoC will use this I guess ? >>> >>> But there are many many devices with this SoC and lots of people are >>> trying to run "plain" Linux on these. >>> >>> I can see how you don't want this as a phy driver though, I would >>> be happy to drop the phy-binding bits (they're not really used >>> anyways) and drop this under drivers/misc adding myself to MAINTAINERS >>> for it, would that be an acceptable solution ? >>> >>> About the "other 2-patch dwc3 series", I'm fine with you not taking >>> the IDA patch, but the first patch is unrelated to IDA, it is a pure >>> bug-fix and really should be upstreamed. >>> >>> Matthias, assuming Felipe is ok with putting this in drivers/misc >>> (minus the phy bindings), are you ok with the xhci blurb which >>> registers the platform-device for the "misc" driver to bind to? >> >> Not the ideal solution, but creating a device would sound better than >> carrying it >> in xhci driver.
[PATCH 1/1] usb: xhci: hold lock over xhci_abort_cmd_ring()
In command timer function, xhci_handle_command_timeout(), xhci->lock
is unlocked before call into xhci_abort_cmd_ring(). This might cause
race between the timer function and the event handler.
The xhci_abort_cmd_ring() function sets the CMD_RING_ABORT bit in the
command register and polling it until the setting takes effect. A stop
command ring event might be handled between writing the abort bit and
polling for it. The event handler will restart the command ring, which
causes the failure of polling, and we ever believed that we failed to
stop it.
As a bonus, this also fixes some issues of calling functions without
locking in xhci_handle_command_timeout().
Cc: # 3.7+
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 797137e..0f99078 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1283,29 +1283,34 @@ void xhci_handle_command_timeout(unsigned long data)
hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
(hw_ring_state & CMD_RING_RUNNING)) {
- spin_unlock_irqrestore(&xhci->lock, flags);
xhci_dbg(xhci, "Command timeout\n");
ret = xhci_abort_cmd_ring(xhci);
if (unlikely(ret == -ESHUTDOWN)) {
xhci_err(xhci, "Abort command ring failed\n");
xhci_cleanup_command_queue(xhci);
+ spin_unlock_irqrestore(&xhci->lock, flags);
usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
xhci_dbg(xhci, "xHCI host controller is dead.\n");
+
+ return;
}
- return;
+
+ goto time_out_completed;
}
/* command ring failed to restart, or host removed. Bail out */
if (second_timeout || xhci->xhc_state & XHCI_STATE_REMOVING) {
- spin_unlock_irqrestore(&xhci->lock, flags);
xhci_dbg(xhci, "command timed out twice, ring start fail?\n");
xhci_cleanup_command_queue(xhci);
- return;
+
+ goto time_out_completed;
}
/* command timeout on stopped ring, ring can't be aborted */
xhci_dbg(xhci, "Command timeout on stopped ring\n");
xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd);
+
+time_out_completed:
spin_unlock_irqrestore(&xhci->lock, flags);
return;
}
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/1] usb: xhci: hold lock over xhci_abort_cmd_ring()
Hi Mathias, This is a follow-up patch for below comment "fix the lock to cover abort+CRR check, and send it to usb-linus +stable" in below discussion thread. https://lkml.org/lkml/2016/12/21/186 It's based on v4.9. Best regards, Lu Baolu Lu Baolu (1): usb: xhci: hold lock over xhci_abort_cmd_ring() drivers/usb/host/xhci-ring.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/1] xhci: Fix race related to abort operation
Hi Mathias, This is a follow-up patch for below comment "rebase OGAWA Hirofumi's changes on top of that, and send to usb-linus only" in below discussion thread. https://lkml.org/lkml/2016/12/21/186 I rebased the patch and added unlock-before and lock-after xhci->lock during waiting for the command stopped event. It sits at the same place in your timeout_race_fixes branch. (git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git timeout_race_fixes) Best regards, Lu Baolu OGAWA Hirofumi (1): xhci: Fix race related to abort operation drivers/usb/host/xhci-mem.c | 1 + drivers/usb/host/xhci-ring.c | 171 ++- drivers/usb/host/xhci.h | 1 + 3 files changed, 91 insertions(+), 82 deletions(-) -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] xhci: Fix race related to abort operation
From: OGAWA Hirofumi
Current abort operation has race.
xhci_handle_command_timeout()
xhci_abort_cmd_ring()
xhci_write_64(CMD_RING_ABORT)
xhci_handshake(5s)
do {
check CMD_RING_RUNNING
udelay(1)
...
COMP_CMD_ABORT event
COMP_CMD_STOP event
xhci_handle_stopped_cmd_ring()
restart cmd_ring
CMD_RING_RUNNING become 1 again
} while ()
return -ETIMEDOUT
xhci_write_64(CMD_RING_ABORT)
/* can abort random command */
To do abort operation correctly, we have to wait both of COMP_CMD_STOP
event and negation of CMD_RING_RUNNING.
But like above, while timeout handler is waiting negation of
CMD_RING_RUNNING, event handler can restart cmd_ring. So timeout
handler never be notice negation of CMD_RING_RUNNING, and retry of
CMD_RING_ABORT can abort random command (BTW, I guess retry of
CMD_RING_ABORT was workaround of this race).
To fix this race, this moves xhci_handle_stopped_cmd_ring() to
xhci_abort_cmd_ring(). And timeout handler waits COMP_CMD_STOP event.
At this point, timeout handler is owner of cmd_ring, and safely
restart cmd_ring by using xhci_handle_stopped_cmd_ring().
[FWIW, as bonus, this way would be easily extend to add CMD_RING_PAUSE
operation]
[baolu: release xhci->lock before wait and apply it back again after
wait. This is the reason my signed-off-by is added.]
CC:
Signed-off-by: OGAWA Hirofumi
Signed-off-by: Lu Baolu
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci-mem.c | 1 +
drivers/usb/host/xhci-ring.c | 171 ++-
drivers/usb/host/xhci.h | 1 +
3 files changed, 91 insertions(+), 82 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 536f572..70c9204 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2379,6 +2379,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
/* init command timeout work */
INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
+ init_completion(&xhci->cmd_ring_stop_completion);
page_size = readl(&xhci->op_regs->page_size);
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 63af013..d5fde9a 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -265,23 +265,71 @@ static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci,
unsigned long delay)
return mod_delayed_work(system_wq, &xhci->cmd_timer, delay);
}
-static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
+static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci)
{
+ return list_first_entry_or_null(&xhci->cmd_list, struct xhci_command,
+ cmd_list);
+}
+
+/*
+ * Turn all commands on command ring with status set to "aborted" to no-op
trbs.
+ * If there are other commands waiting then restart the ring and kick the
timer.
+ * This must be called with command ring stopped and xhci->lock held.
+ */
+static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
+struct xhci_command *cur_cmd)
+{
+ struct xhci_command *i_cmd;
+ u32 cycle_state;
+
+ /* Turn all aborted commands in list to no-ops, then restart */
+ list_for_each_entry(i_cmd, &xhci->cmd_list, cmd_list) {
+
+ if (i_cmd->status != COMP_CMD_ABORT)
+ continue;
+
+ i_cmd->status = COMP_CMD_STOP;
+
+ xhci_dbg(xhci, "Turn aborted command %p to no-op\n",
+i_cmd->command_trb);
+ /* get cycle state from the original cmd trb */
+ cycle_state = le32_to_cpu(
+ i_cmd->command_trb->generic.field[3]) & TRB_CYCLE;
+ /* modify the command trb to no-op command */
+ i_cmd->command_trb->generic.field[0] = 0;
+ i_cmd->command_trb->generic.field[1] = 0;
+ i_cmd->command_trb->generic.field[2] = 0;
+ i_cmd->command_trb->generic.field[3] = cpu_to_le32(
+ TRB_TYPE(TRB_CMD_NOOP) | cycle_state);
+
+ /*
+* caller waiting for completion is called when command
+* completion event is received for these no-op commands
+*/
+ }
+
+ xhci->cmd_ring_state = CMD_RING_STATE_RUNNING;
+
+ /* ring command ring doorbell to restart the command ring */
+ if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
[PATCH 4/4] usb: xhci: warn on command timeout in stopped command ring
If xhci host fails to response to a command, the command watchdog timer will be fired. The callback function will abort and clear current command and restart the command execution. If driver fails to restart command execution, it will assume there is a larger problem in host and report the situation to the upper layer. In rare cases, if the driver sees a command timeout in a stopped command ring, driver should let the user know it. Signed-off-by: Lu Baolu --- drivers/usb/host/xhci-ring.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 6a23c37..16baefc 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -1308,8 +1308,12 @@ void xhci_handle_command_timeout(struct work_struct *work) goto time_out_completed; } - /* command timeout on stopped ring, ring can't be aborted */ - xhci_dbg(xhci, "Command timeout on stopped ring\n"); + /* +* We should never reach here until a command times out on a stopped +* ring and xhci driver has no idea about the reason why the command +* ring was stopped. +*/ + xhci_warn(xhci, "WARN command timeout on stopped ring\n"); xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd); time_out_completed: -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] refactor command timeout handling
Hi Mathias, This patch series is for command timeout refactoring. Patches usb: xhci: remove unnecessary second abort try usb: xhci: remove CRR polling in xhci_abort_cmd_ring() are follow-ups of your comments of "remove unnecessary second abort try as a separate patch, send to usb-next" and "remove polling for the Command ring running (CRR), waiting for completion is enough, if completion times out then we can check CRR. for usb-next" in below discussion thread. https://lkml.org/lkml/2016/12/21/186 Patches usb: xhci: add XHCI_MISS_CA_EVENT quirk bit usb: xhci: warn on command timeout in stopped command ring are my proposals. They base on the top of your timeout_race_fixes branch. (git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git timeout_race_fixes) Lu Baolu (4): usb: xhci: remove unnecessary second abort try usb: xhci: remove CRR polling in xhci_abort_cmd_ring() usb: xhci: add XHCI_MISS_CA_EVENT quirk bit usb: xhci: warn on command timeout in stopped command ring drivers/usb/host/xhci-ring.c | 64 ++-- drivers/usb/host/xhci.h | 6 + 2 files changed, 32 insertions(+), 38 deletions(-) -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] usb: xhci: remove unnecessary second abort try
The second try was a workaround for (what we thought was) command
ring failing to stop in the first place. But this turns out to be
due to the race that we have fixed(see "xhci: Fix race related to
abort operation"). With that fix, it is time to remove the second
try.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 18 +-
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 2f4ea21..e3bcf6d 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -343,19 +343,11 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci,
unsigned long *flags)
ret = xhci_handshake(&xhci->op_regs->cmd_ring,
CMD_RING_RUNNING, 0, 5 * 1000 * 1000);
if (ret < 0) {
- /* we are about to kill xhci, give it one more chance */
- xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
- &xhci->op_regs->cmd_ring);
- udelay(1000);
- ret = xhci_handshake(&xhci->op_regs->cmd_ring,
-CMD_RING_RUNNING, 0, 3 * 1000 * 1000);
- if (ret < 0) {
- xhci_err(xhci, "Stopped the command ring failed, "
-"maybe the host is dead\n");
- xhci->xhc_state |= XHCI_STATE_DYING;
- xhci_halt(xhci);
- return -ESHUTDOWN;
- }
+ xhci_err(xhci,
+"Stop command ring failed, maybe the host is dead\n");
+ xhci->xhc_state |= XHCI_STATE_DYING;
+ xhci_halt(xhci);
+ return -ESHUTDOWN;
}
/*
* Writing the CMD_RING_ABORT bit should cause a cmd completion event,
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] usb: xhci: remove CRR polling in xhci_abort_cmd_ring()
xHCI driver aborts the command ring by setting CA (Command
Abort) bit in the command register. With this setting, host
should abort all command executions and generate a command
completion event with the completion code set to command
ring stopped. Software should time the completion of command
abort by checking the CRR (Command Ring Running) and waiting
for the command ring stopped event.
Current xhci_abort_cmd_ring() does this in the following
way: setting CA bit; busy polling CRR bit until it negates;
sleep and wait for the command ring stopped event. This is
not an efficient way since cpu cycles are wasted on polling
registers. This patch removes polling for CRR (Command Ring
Running). Wait for completion, and check CRR if completion
times out is enough.
Suggested-by: Mathias Nyman
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 49 ++--
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index e3bcf6d..5935dce 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -323,7 +323,6 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci,
unsigned long *flags)
{
unsigned long timeleft;
u64 temp_64;
- int ret;
xhci_dbg(xhci, "Abort command ring\n");
@@ -333,34 +332,34 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci,
unsigned long *flags)
xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
&xhci->op_regs->cmd_ring);
- /* Section 4.6.1.2 of xHCI 1.0 spec says software should
-* time the completion od all xHCI commands, including
-* the Command Abort operation. If software doesn't see
-* CRR negated in a timely manner (e.g. longer than 5
-* seconds), then it should assume that the there are
-* larger problems with the xHC and assert HCRST.
-*/
- ret = xhci_handshake(&xhci->op_regs->cmd_ring,
- CMD_RING_RUNNING, 0, 5 * 1000 * 1000);
- if (ret < 0) {
- xhci_err(xhci,
-"Stop command ring failed, maybe the host is dead\n");
- xhci->xhc_state |= XHCI_STATE_DYING;
- xhci_halt(xhci);
- return -ESHUTDOWN;
- }
- /*
-* Writing the CMD_RING_ABORT bit should cause a cmd completion event,
-* however on some host hw the CMD_RING_RUNNING bit is correctly cleared
-* but the completion event in never sent. Wait 2 secs (arbitrary
-* number) to handle those cases after negation of CMD_RING_RUNNING.
-*/
spin_unlock_irqrestore(&xhci->lock, *flags);
timeleft = wait_for_completion_timeout(&xhci->cmd_ring_stop_completion,
- 2 * HZ);
+ XHCI_CMD_DEFAULT_TIMEOUT);
spin_lock_irqsave(&xhci->lock, *flags);
if (!timeleft) {
- xhci_dbg(xhci, "No stop event for abort, ring start fail?\n");
+ /* Section 4.6.1.2 of xHCI 1.0 spec says software should
+* time the completion of all xHCI commands, including
+* the Command Abort operation. If software doesn't see
+* CRR negated in a timely manner (e.g. longer than 5
+* seconds), then it should assume that the there are
+* larger problems with the xHC and assert HCRST.
+*/
+ temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
+ if (temp_64 & CMD_RING_RUNNING) {
+ xhci_err(xhci,
+"Stop command ring failed, maybe the host is
dead\n");
+ xhci->xhc_state |= XHCI_STATE_DYING;
+ xhci_halt(xhci);
+ return -ESHUTDOWN;
+ }
+
+ /*
+* Writing the CMD_RING_ABORT bit should cause a cmd
+* completion event, however on some hosts the bit is
+* correctly cleared but the completion event is never
+* sent.
+*/
+ xhci_warn(xhci, "No stop event for abort, ring start fail?\n");
xhci_cleanup_command_queue(xhci);
} else {
xhci_handle_stopped_cmd_ring(xhci, xhci_next_queued_cmd(xhci));
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] usb: xhci: add XHCI_MISS_CA_EVENT quirk bit
Writing the CMD_RING_ABORT bit in xhci command register should
cause a command completion event with the command completion
code set to command ring stopped. However on some hosts, the
CMD_RING_RUNNING bit is correctly cleared but the completion
event is never sent.
Current xhci driver treats the behavior of "CMD_RING_RUNNING
bit is correctly cleared but the completion event is missed"
as a correct behavior for all hosts. This is different from
that defined in xhci spec. Refer to 4.6.1.2 in xhci spec.
This patch introduces a quirk bit for those quirky hardwares.
For normal hosts, if the command completion for abort command
ring misses, we shall assume that there are larger problems
with the host and assert HCRST.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 17 +
drivers/usb/host/xhci.h | 6 ++
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 5935dce..6a23c37 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -345,25 +345,18 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci,
unsigned long *flags)
* larger problems with the xHC and assert HCRST.
*/
temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
- if (temp_64 & CMD_RING_RUNNING) {
+ if ((temp_64 & CMD_RING_RUNNING) ||
+ !(xhci->quirks & XHCI_MISS_CA_EVENT)) {
xhci_err(xhci,
"Stop command ring failed, maybe the host is
dead\n");
xhci->xhc_state |= XHCI_STATE_DYING;
xhci_halt(xhci);
return -ESHUTDOWN;
}
-
- /*
-* Writing the CMD_RING_ABORT bit should cause a cmd
-* completion event, however on some hosts the bit is
-* correctly cleared but the completion event is never
-* sent.
-*/
- xhci_warn(xhci, "No stop event for abort, ring start fail?\n");
- xhci_cleanup_command_queue(xhci);
- } else {
- xhci_handle_stopped_cmd_ring(xhci, xhci_next_queued_cmd(xhci));
}
+
+ xhci_handle_stopped_cmd_ring(xhci, xhci_next_queued_cmd(xhci));
+
return 0;
}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 09fe63f..c550bf0 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1656,6 +1656,12 @@ struct xhci_hcd {
#define XHCI_SSIC_PORT_UNUSED (1 << 22)
#define XHCI_NO_64BIT_SUPPORT (1 << 23)
#define XHCI_MISSING_CAS (1 << 24)
+/*
+ * Writing the CMD_RING_ABORT bit should cause a cmd completion event,
+ * however on some host hw the CMD_RING_RUNNING bit is correctly cleared
+ * but the completion event is never sent.
+ */
+#define XHCI_MISS_CA_EVENT (1 << 25)
unsigned intnum_active_eps;
unsigned intlimit_active_eps;
/* There are two roothubs to keep track of bus suspend info for */
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler
Hi,
On 12/26/2016 04:01 PM, Baolin Wang wrote:
> On some platfroms(like x86 platform), when one core is running the USB gadget
> irq thread handler by dwc3_thread_interrupt(), meanwhile another core also can
> respond other interrupts from dwc3 controller and modify the event buffer by
> dwc3_interrupt() function, that will cause getting the wrong event count in
> irq thread handler to make the USB function abnormal.
>
> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this race.
Why not spin_lock_irq ones? This lock seems to be used in both
normal and interrupt threads. Or, I missed anything?
Best regards,
Lu Baolu
>
> Signed-off-by: Baolin Wang
> ---
> drivers/usb/dwc3/gadget.c |6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6785595..1a1e1f4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2894,10 +2894,13 @@ static irqreturn_t dwc3_check_event_buf(struct
> dwc3_event_buffer *evt)
> return IRQ_HANDLED;
> }
>
> + spin_lock(&dwc->lock);
> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> count &= DWC3_GEVNTCOUNT_MASK;
> - if (!count)
> + if (!count) {
> + spin_unlock(&dwc->lock);
> return IRQ_NONE;
> + }
>
> evt->count = count;
> evt->flags |= DWC3_EVENT_PENDING;
> @@ -2914,6 +2917,7 @@ static irqreturn_t dwc3_check_event_buf(struct
> dwc3_event_buffer *evt)
> memcpy(evt->cache, evt->buf, count - amount);
>
> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
> + spin_unlock(&dwc->lock);
>
> return IRQ_WAKE_THREAD;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler
Hi, On 12/27/2016 10:58 AM, Baolin Wang wrote: > Hi, > > On 27 December 2016 at 10:39, Lu Baolu wrote: >> Hi, >> >> On 12/26/2016 04:01 PM, Baolin Wang wrote: >>> On some platfroms(like x86 platform), when one core is running the USB >>> gadget >>> irq thread handler by dwc3_thread_interrupt(), meanwhile another core also >>> can >>> respond other interrupts from dwc3 controller and modify the event buffer by >>> dwc3_interrupt() function, that will cause getting the wrong event count in >>> irq thread handler to make the USB function abnormal. >>> >>> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this >>> race. >> Why not spin_lock_irq ones? This lock seems to be used in both >> normal and interrupt threads. Or, I missed anything? > I assumed there are no nested interrupts, when one core is running at > interrupt context, then it can not respond any other interrupts, which > means we don't need to disable local IRQ now, right? > Fair enough. Thanks. Best regards, Lu Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/37] usb: host: xhci: WARN on unexpected COMP_SUCCESS
Hi,
On 12/29/2016 07:00 PM, Felipe Balbi wrote:
> COMP_SUCCESS should only be asserted on a *true* sucessful transfer. Any
> other cases are bogus and we should aim to catch them. One easy way to
> get bug reports is to trigger a WARN_ONCE() on such cases.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/host/xhci-ring.c | 28 +++-
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 17b878b53b46..772e07e2ef16 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1934,6 +1934,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci,
> struct xhci_td *td,
> {
> struct xhci_virt_device *xdev;
> struct xhci_ring *ep_ring;
> + struct device *dev;
> unsigned int slot_id;
> int ep_index;
> struct xhci_ep_ctx *ep_ctx;
> @@ -1950,15 +1951,15 @@ static int process_ctrl_td(struct xhci_hcd *xhci,
> struct xhci_td *td,
> trb_comp_code = GET_COMP_CODE(le32_to_cpu(event->transfer_len));
> requested = td->urb->transfer_buffer_length;
> remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
> + dev = xhci_to_hcd(xhci)->self.controller;
>
> switch (trb_comp_code) {
> case COMP_SUCCESS:
> - if (trb_type != TRB_STATUS) {
> - xhci_warn(xhci, "WARN: Success on ctrl %s TRB without
> IOC set?\n",
> - (trb_type == TRB_DATA) ? "data" :
> "setup");
> - *status = -ESHUTDOWN;
> - break;
> - }
> + dev_WARN_ONCE(dev, trb_type != TRB_STATUS,
> + "ep%d%s: unexpected success! TRB Type %d\n",
> + usb_endpoint_num(&td->urb->ep->desc),
> + usb_endpoint_dir_in(&td->urb->ep->desc) ?
> + "in" : "out", trb_type);
> *status = 0;
Still setting status to 0 even "trb_type != TRB_STATUS"?
Previous code set it to -ESHUTDOWN.
> break;
> case COMP_SHORT_PACKET:
> @@ -2150,6 +2151,7 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci,
> struct xhci_td *td,
> struct xhci_virt_ep *ep, int *status)
> {
> struct xhci_ring *ep_ring;
> + struct device *dev;
> u32 trb_comp_code;
> u32 remaining, requested, ep_trb_len;
>
> @@ -2158,16 +2160,16 @@ static int process_bulk_intr_td(struct xhci_hcd
> *xhci, struct xhci_td *td,
> remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
> ep_trb_len = TRB_LEN(le32_to_cpu(ep_trb->generic.field[2]));
> requested = td->urb->transfer_buffer_length;
> + dev = xhci_to_hcd(xhci)->self.controller;
>
> switch (trb_comp_code) {
> case COMP_SUCCESS:
> - /* handle success with untransferred data as short packet */
> - if (ep_trb != td->last_trb || remaining) {
> - xhci_warn(xhci, "WARN Successful completion on short
> TX\n");
> - xhci_dbg(xhci, "ep %#x - asked for %d bytes, %d bytes
> untransferred\n",
> - td->urb->ep->desc.bEndpointAddress,
> - requested, remaining);
> - }
> + dev_WARN_ONCE(dev, (ep_trb != td->last_trb) || remaining,
> + "ep%d%s: unexpected success! TRB %p/%p size
> %d/%d\n",
> + usb_endpoint_num(&td->urb->ep->desc),
> + usb_endpoint_dir_in(&td->urb->ep->desc) ?
> + "in" : "out", ep_trb, td->last_trb, remaining,
> + requested);
> *status = 0;
The same consideration here.
Best regards,
Lu Baolu
> break;
> case COMP_SHORT_PACKET:
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/37] usb: host: xhci: clear only STS_EINT
Hi,
On 12/29/2016 07:00 PM, Felipe Balbi wrote:
> Many other bits in USBSTS register are "clear-by-writing-1". Let's make
> sure that we clear *only* STS_EINT and not any of the other bits as they
> might be needed later.
Bit 13~31 in status register is for RsvdP (Reserved and Preserved).
This means these bits are reserved for future RW implementations.
Software shall preserve the value read for writes.
How about below helper?
static inline void clear_status_rw1c_bit(u32 bit)
{
u32 status;
status = readl(&xhci->op_regs->status);
/* preserve RsvP bits and clear RO/RW1C/RsvZ bits */
status &= ~0x1fff;
status |= bit;
writel(status, &xhci->op_regs->status);
}
Look into the code, I still find other two places where RW1C bits
are not cleared correctly.
In xhci_stop() and xhci_resume(), the RW1C bit is treated as a RW type.
724 temp = readl(&xhci->op_regs->status);
725 writel(temp & ~STS_EINT, &xhci->op_regs->status);
I will correct these in a separated patch and cc stable as well.
Best regards,
Lu Baolu
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/host/xhci-ring.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index b4ec80d18078..116b4a0dadbb 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2630,8 +2630,7 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
>* so we can receive interrupts from other MSI-X interrupters.
>* Write 1 to clear the interrupt status.
>*/
> - status |= STS_EINT;
> - writel(status, &xhci->op_regs->status);
> + writel(STS_EINT, &xhci->op_regs->status);
> /* FIXME when MSI-X is supported and there are multiple vectors */
> /* Clear the MSI-X event interrupt status */
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 24/37] usb: host: xhci: use trb_to_noop() from xhci_handle_stopped_cmd_ring()
Hi,
On 12/29/2016 07:00 PM, Felipe Balbi wrote:
> instead of open coding how to convert a TRB to no-op, let's use our
> newly introduced helper.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/host/xhci-ring.c | 33 +++--
> 1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index f369d97f663d..0b8f728d6e77 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -542,6 +542,27 @@ static void trb_to_noop(union xhci_trb *trb)
> trb->generic.field[3] &= cpu_to_le32(TRB_CYCLE);
> trb->generic.field[3] |= cpu_to_le32(TRB_TYPE(TRB_TR_NOOP));
> break;
> + case TRB_ENABLE_SLOT:
> + case TRB_DISABLE_SLOT:
> + case TRB_ADDR_DEV:
> + case TRB_CONFIG_EP:
> + case TRB_EVAL_CONTEXT:
> + case TRB_RESET_EP:
> + case TRB_STOP_RING:
> + case TRB_SET_DEQ:
> + case TRB_RESET_DEV:
> + case TRB_FORCE_EVENT:
> + case TRB_NEG_BANDWIDTH:
> + case TRB_SET_LT:
> + case TRB_GET_BW:
> + case TRB_FORCE_HEADER:
How about merging
+ case TRB_ENABLE_SLOT:
+ case TRB_DISABLE_SLOT:
+ case TRB_ADDR_DEV:
+ case TRB_CONFIG_EP:
+ case TRB_EVAL_CONTEXT:
+ case TRB_RESET_EP:
+ case TRB_STOP_RING:
+ case TRB_SET_DEQ:
+ case TRB_RESET_DEV:
+ case TRB_FORCE_EVENT:
+ case TRB_NEG_BANDWIDTH:
+ case TRB_SET_LT:
+ case TRB_GET_BW:
+ case TRB_FORCE_HEADER:
into
+case TRB_ENABLE_SLOT ... TRB_FORCE_HEADER:
?
> + trb->generic.field[0] = 0;
> + trb->generic.field[1] = 0;
> + trb->generic.field[2] = 0;
> + /* Preserve only the cycle bit of this TRB */
> + trb->generic.field[3] &= cpu_to_le32(TRB_CYCLE);
> + trb->generic.field[3] = cpu_to_le32(TRB_TYPE(TRB_CMD_NOOP));
> + break;
> default:
> /* nothing */
Need a warning?
Best regards,
Lu Baolu
> break;
> @@ -1229,7 +1250,6 @@ static void xhci_handle_stopped_cmd_ring(struct
> xhci_hcd *xhci,
>struct xhci_command *cur_cmd)
> {
> struct xhci_command *cmd;
> - u32 cycle_state;
>
> /* Turn all aborted commands in list to no-ops, then restart */
> list_for_each_entry(cmd, &xhci->cmd_list,
> @@ -1242,15 +1262,8 @@ static void xhci_handle_stopped_cmd_ring(struct
> xhci_hcd *xhci,
>
> xhci_dbg(xhci, "Turn aborted command %p to no-op\n",
>cmd->command_trb);
> - /* get cycle state from the original cmd trb */
> - cycle_state = le32_to_cpu(
> - cmd->command_trb->generic.field[3]) & TRB_CYCLE;
> - /* modify the command trb to no-op command */
> - cmd->command_trb->generic.field[0] = 0;
> - cmd->command_trb->generic.field[1] = 0;
> - cmd->command_trb->generic.field[2] = 0;
> - cmd->command_trb->generic.field[3] = cpu_to_le32(
> - TRB_TYPE(TRB_CMD_NOOP) | cycle_state);
> +
> + trb_to_noop(cmd->command_trb);
>
> /*
>* caller waiting for completion is called when command
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 30/37] usb: host: xhci: make a generic TRB tracer
(unsigned long long) __entry->dma, __entry->va,
> - __entry->status, __entry->flags
> + TP_printk("%s: %s", xhci_ring_type_string(__entry->type),
> + xhci_decode_trb(__entry->field0, __entry->field1,
> + __entry->field2, __entry->field3)
> )
> );
>
> -DEFINE_EVENT(xhci_log_event, xhci_cmd_completion,
> - TP_PROTO(void *trb_va, struct xhci_generic_trb *ev),
> - TP_ARGS(trb_va, ev)
> +DEFINE_EVENT(xhci_log_trb, xhci_handle_event,
> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
> + TP_ARGS(ring, trb)
> +);
> +
> +DEFINE_EVENT(xhci_log_trb, xhci_handle_command,
> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
> + TP_ARGS(ring, trb)
> +);
> +
> +DEFINE_EVENT(xhci_log_trb, xhci_handle_transfer,
> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
> + TP_ARGS(ring, trb)
> +);
> +
> +DEFINE_EVENT(xhci_log_trb, xhci_queue_trb,
> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
> + TP_ARGS(ring, trb)
> );
How about add xhci_dequeue_trb for those dequeued from event ring?
It should be helpful, if we can track the link trb as well.
Best regards,
Lu Baolu
>
> #endif /* __XHCI_TRACE_H */
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index c7e95a6e39a7..eb72ad4c0d72 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1200,6 +1200,27 @@ struct xhci_event_cmd {
>
> /* Address device - disable SetAddress */
> #define TRB_BSR (1<<9)
> +
> +/* Configure Endpoint - Deconfigure */
> +#define TRB_DC (1<<9)
> +
> +/* Stop Ring - Transfer State Preserve */
> +#define TRB_TSP (1<<9)
> +
> +/* Force Event */
> +#define TRB_TO_VF_INTR_TARGET(p) (((p) & (0x3ff << 22)) >> 22)
> +#define TRB_TO_VF_ID(p) (((p) & (0xff << 16)) >> 16)
> +
> +/* Set Latency Tolerance Value */
> +#define TRB_TO_BELT(p) (((p) & (0xfff << 16)) >> 16)
> +
> +/* Get Port Bandwidth */
> +#define TRB_TO_DEV_SPEED(p) (((p) & (0xf << 16)) >> 16)
> +
> +/* Force Header */
> +#define TRB_TO_PACKET_TYPE(p)((p) & 0x1f)
> +#define TRB_TO_ROOTHUB_PORT(p) (((p) & (0xff << 24)) >> 24)
> +
> enum xhci_setup_dev {
> SETUP_CONTEXT_ONLY,
> SETUP_CONTEXT_ADDRESS,
> @@ -1223,16 +1244,21 @@ enum xhci_setup_dev {
> #define STREAM_ID_FOR_TRB(p) p)) & 0x) << 16)
> #define SCT_FOR_TRB(p) (((p) << 1) & 0x7)
>
> +/* Link TRB specific fields */
> +#define TRB_TC (1<<1)
>
> /* Port Status Change Event TRB fields */
> /* Port ID - bits 31:24 */
> #define GET_PORT_ID(p) (((p) & (0xff << 24)) >> 24)
>
> +#define EVENT_DATA (1 << 2)
> +
> /* Normal TRB fields */
> /* transfer_len bitmasks - bits 0:16 */
> #define TRB_LEN(p) ((p) & 0x1)
> /* TD Size, packets remaining in this TD, bits 21:17 (5 bits, so max 31) */
> #define TRB_TD_SIZE(p) (min((p), (u32)31) << 17)
> +#define GET_TD_SIZE(p) (((p) & 0x3e) >> 17)
> /* xhci 1.1 uses the TD_SIZE field for TBC if Extended TBC is enabled (ETE)
> */
> #define TRB_TD_SIZE_TBC(p) (min((p), (u32)31) << 17)
> /* Interrupter Target - which MSI-X vector to target the completion event at
> */
> @@ -1360,6 +1386,80 @@ union xhci_trb {
> /* Get NEC firmware revision. */
> #define TRB_NEC_GET_FW 49
>
> +static inline const char *xhci_trb_type_string(u8 type)
> +{
> + switch (type) {
> + case TRB_NORMAL:
> + return "Normal";
> + case TRB_SETUP:
> + return "Setup Stage";
> + case TRB_DATA:
> + return "Data Stage";
> + case TRB_STATUS:
> + return "Status Stage";
> + case TRB_ISOC:
> + return "Isoch";
> + case TRB_LINK:
> + return "Link";
> + case TRB_EVENT_DATA:
> + return "Event Data";
> + case TRB_TR_NOOP:
> + return "No-Op";
> + case TRB_ENABLE_SLOT:
> + return "Enable Slot Command";
> + case TRB_DISABLE_SLOT:
> + return "Disable Slot Command";
> + case TRB_ADDR_DEV:
> +
[PATCH 1/1] usb: xhci: clear EINT bit in status correctly
EINT(Event Interrupt) is a write-1-to-clear type of bit in xhci status register. It should be cleared by writing a 1. Writing 0 to this bit has no effect. Xhci driver tries to clear this bit by writing 0 to it. This is not the right way to go. This patch corrects this by reading the register first, then clearing all RO/RW1C/RsvZ bits and setting the clearing bit, and writing back the new value at last. Xhci spec requires that software that uses EINT shall clear it prior to clearing any IP flags in section 5.4.2. This is the reason why this patch is CC'ed stable as well. Cc: # v3.14+ CC: Felipe Balbi Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 1cd5641..18ea6b8 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -721,7 +721,7 @@ void xhci_stop(struct usb_hcd *hcd) xhci_dbg_trace(xhci, trace_xhci_dbg_init, "// Disabling event ring interrupts"); temp = readl(&xhci->op_regs->status); - writel(temp & ~STS_EINT, &xhci->op_regs->status); + writel(temp & ~0x1fff | STS_EINT, &xhci->op_regs->status); temp = readl(&xhci->ir_set->irq_pending); writel(ER_IRQ_DISABLE(temp), &xhci->ir_set->irq_pending); xhci_print_ir_set(xhci, 0); @@ -1054,7 +1054,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated) xhci_dbg(xhci, "// Disabling event ring interrupts\n"); temp = readl(&xhci->op_regs->status); - writel(temp & ~STS_EINT, &xhci->op_regs->status); + writel(temp & ~0x1fff | STS_EINT, &xhci->op_regs->status); temp = readl(&xhci->ir_set->irq_pending); writel(ER_IRQ_DISABLE(temp), &xhci->ir_set->irq_pending); xhci_print_ir_set(xhci, 0); -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/37] usb: host: xhci: clear only STS_EINT
Hi,
On 12/30/2016 03:52 PM, Felipe Balbi wrote:
> Hi,
>
> Lu Baolu writes:
>> On 12/29/2016 07:00 PM, Felipe Balbi wrote:
>>> Many other bits in USBSTS register are "clear-by-writing-1". Let's make
>>> sure that we clear *only* STS_EINT and not any of the other bits as they
>>> might be needed later.
>> Bit 13~31 in status register is for RsvdP (Reserved and Preserved).
> totally missed the "Preserved" part. Thanks
>
>> This means these bits are reserved for future RW implementations.
>> Software shall preserve the value read for writes.
>>
>> How about below helper?
>>
>> static inline void clear_status_rw1c_bit(u32 bit)
>> {
>> u32 status;
>>
>> status = readl(&xhci->op_regs->status);
>> /* preserve RsvP bits and clear RO/RW1C/RsvZ bits */
>> status &= ~0x1fff;
>> status |= bit;
>>
>> writel(status, &xhci->op_regs->status);
>> }
> not sure, I have no problems with it, but it's for mathias to decide. If
> anything, it should be prefixed by xhci_ so it's easier to use ftrace.
>
>> Look into the code, I still find other two places where RW1C bits
>> are not cleared correctly.
>>
>> In xhci_stop() and xhci_resume(), the RW1C bit is treated as a RW type.
>>
>> 724 temp = readl(&xhci->op_regs->status);
>> 725 writel(temp & ~STS_EINT, &xhci->op_regs->status);
>>
>> I will correct these in a separated patch and cc stable as well.
> That's cool. Then I can drop this patch from my series.
You don't need to drop this patch from your series. My patch
didn't include your change.
Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 24/37] usb: host: xhci: use trb_to_noop() from xhci_handle_stopped_cmd_ring()
Hi, On 12/30/2016 03:54 PM, Felipe Balbi wrote: > Hi, > > Lu Baolu writes: >> On 12/29/2016 07:00 PM, Felipe Balbi wrote: >>> instead of open coding how to convert a TRB to no-op, let's use our >>> newly introduced helper. >>> >>> Signed-off-by: Felipe Balbi >>> --- >>> drivers/usb/host/xhci-ring.c | 33 +++-- >>> 1 file changed, 23 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >>> index f369d97f663d..0b8f728d6e77 100644 >>> --- a/drivers/usb/host/xhci-ring.c >>> +++ b/drivers/usb/host/xhci-ring.c >>> @@ -542,6 +542,27 @@ static void trb_to_noop(union xhci_trb *trb) >>> trb->generic.field[3] &= cpu_to_le32(TRB_CYCLE); >>> trb->generic.field[3] |= cpu_to_le32(TRB_TYPE(TRB_TR_NOOP)); >>> break; >>> + case TRB_ENABLE_SLOT: >>> + case TRB_DISABLE_SLOT: >>> + case TRB_ADDR_DEV: >>> + case TRB_CONFIG_EP: >>> + case TRB_EVAL_CONTEXT: >>> + case TRB_RESET_EP: >>> + case TRB_STOP_RING: >>> + case TRB_SET_DEQ: >>> + case TRB_RESET_DEV: >>> + case TRB_FORCE_EVENT: >>> + case TRB_NEG_BANDWIDTH: >>> + case TRB_SET_LT: >>> + case TRB_GET_BW: >>> + case TRB_FORCE_HEADER: >> How about merging >> >> +case TRB_ENABLE_SLOT: >> +case TRB_DISABLE_SLOT: >> +case TRB_ADDR_DEV: >> +case TRB_CONFIG_EP: >> +case TRB_EVAL_CONTEXT: >> +case TRB_RESET_EP: >> +case TRB_STOP_RING: >> +case TRB_SET_DEQ: >> +case TRB_RESET_DEV: >> +case TRB_FORCE_EVENT: >> +case TRB_NEG_BANDWIDTH: >> +case TRB_SET_LT: >> +case TRB_GET_BW: >> +case TRB_FORCE_HEADER: >> >> into >> >> >> +case TRB_ENABLE_SLOT ... TRB_FORCE_HEADER: >> >> ? > to me it's pointless obfuscation :-) > >> >> >>> + trb->generic.field[0] = 0; >>> + trb->generic.field[1] = 0; >>> + trb->generic.field[2] = 0; >>> + /* Preserve only the cycle bit of this TRB */ >>> + trb->generic.field[3] &= cpu_to_le32(TRB_CYCLE); >>> + trb->generic.field[3] = cpu_to_le32(TRB_TYPE(TRB_CMD_NOOP)); >>> + break; >>> default: >>> /* nothing */ >> Need a warning? > We have two users for this function and we know we won't pass unexistent > TRB types to it. Maybe we can defer that warning until a problem shows up? > Fair enough. Best regards, Lu Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 30/37] usb: host: xhci: make a generic TRB tracer
Hi, On 12/30/2016 03:47 PM, Felipe Balbi wrote: > Hi, > > Lu Baolu writes: >>> +DEFINE_EVENT(xhci_log_trb, xhci_handle_event, >>> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb), >>> + TP_ARGS(ring, trb) >>> +); >>> + >>> +DEFINE_EVENT(xhci_log_trb, xhci_handle_command, >>> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb), >>> + TP_ARGS(ring, trb) >>> +); >>> + >>> +DEFINE_EVENT(xhci_log_trb, xhci_handle_transfer, >>> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb), >>> + TP_ARGS(ring, trb) >>> +); >>> + >>> +DEFINE_EVENT(xhci_log_trb, xhci_queue_trb, >>> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb), >>> + TP_ARGS(ring, trb) >>> ); >> How about add xhci_dequeue_trb for those dequeued from event ring? > Sure, and where would the tracer be? When do we "dequeue" TRBs? How about xhci_handle_event()? TRBs are dequeued from the event ring there. Best regards, Lu Baolu > >> It should be helpful, if we can track the link trb as well. > right > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/37] usb: host: xhci: clear only STS_EINT
Hi,
On 01/02/2017 04:47 PM, Felipe Balbi wrote:
> Hi,
>
> Lu Baolu writes:
>> Hi,
>>
>> On 12/30/2016 03:52 PM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> Lu Baolu writes:
>>>> On 12/29/2016 07:00 PM, Felipe Balbi wrote:
>>>>> Many other bits in USBSTS register are "clear-by-writing-1". Let's make
>>>>> sure that we clear *only* STS_EINT and not any of the other bits as they
>>>>> might be needed later.
>>>> Bit 13~31 in status register is for RsvdP (Reserved and Preserved).
>>> totally missed the "Preserved" part. Thanks
>>>
>>>> This means these bits are reserved for future RW implementations.
>>>> Software shall preserve the value read for writes.
>>>>
>>>> How about below helper?
>>>>
>>>> static inline void clear_status_rw1c_bit(u32 bit)
>>>> {
>>>> u32 status;
>>>>
>>>> status = readl(&xhci->op_regs->status);
>>>> /* preserve RsvP bits and clear RO/RW1C/RsvZ bits */
>>>> status &= ~0x1fff;
>>>> status |= bit;
>>>>
>>>> writel(status, &xhci->op_regs->status);
>>>> }
>>> not sure, I have no problems with it, but it's for mathias to decide. If
>>> anything, it should be prefixed by xhci_ so it's easier to use ftrace.
>>>
>>>> Look into the code, I still find other two places where RW1C bits
>>>> are not cleared correctly.
>>>>
>>>> In xhci_stop() and xhci_resume(), the RW1C bit is treated as a RW type.
>>>>
>>>> 724 temp = readl(&xhci->op_regs->status);
>>>> 725 writel(temp & ~STS_EINT, &xhci->op_regs->status);
>>>>
>>>> I will correct these in a separated patch and cc stable as well.
>>> That's cool. Then I can drop this patch from my series.
>> You don't need to drop this patch from your series. My patch
>> didn't include your change.
> Fair enough, thanks :-) So this still needs an update nevertheless,
> right ?
>
Yes.
Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 30/37] usb: host: xhci: make a generic TRB tracer
Hi, On 01/02/2017 04:53 PM, Felipe Balbi wrote: > Hi, > > Lu Baolu writes: >> Hi, >> >> On 12/30/2016 03:47 PM, Felipe Balbi wrote: >>> Hi, >>> >>> Lu Baolu writes: >>>>> +DEFINE_EVENT(xhci_log_trb, xhci_handle_event, >>>>> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb), >>>>> + TP_ARGS(ring, trb) >>>>> +); >>>>> + >>>>> +DEFINE_EVENT(xhci_log_trb, xhci_handle_command, >>>>> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb), >>>>> + TP_ARGS(ring, trb) >>>>> +); >>>>> + >>>>> +DEFINE_EVENT(xhci_log_trb, xhci_handle_transfer, >>>>> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb), >>>>> + TP_ARGS(ring, trb) >>>>> +); >>>>> + >>>>> +DEFINE_EVENT(xhci_log_trb, xhci_queue_trb, >>>>> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb), >>>>> + TP_ARGS(ring, trb) >>>>> ); >>>> How about add xhci_dequeue_trb for those dequeued from event ring? >>> Sure, and where would the tracer be? When do we "dequeue" TRBs? >> How about xhci_handle_event()? TRBs are dequeued from the event >> ring there. > we already have a tracer there ;-) > > @@ -2475,6 +2480,8 @@ static int xhci_handle_event(struct xhci_hcd *xhci) > xhci->event_ring->cycle_state) > return 0; > > + trace_xhci_handle_event(xhci->event_ring, &event->generic); > + > /* > * Barrier between reading the TRB_CYCLE (valid) flag above and any > * speculative reads of the event's flags/data below. > > We also have one for cmd completion: > > @@ -1346,6 +1346,9 @@ static void handle_cmd_completion(struct xhci_hcd *xhci, > > cmd_dma = le64_to_cpu(event->cmd_trb); > cmd_trb = xhci->cmd_ring->dequeue; > + > + trace_xhci_handle_command(xhci->cmd_ring, &cmd_trb->generic); > + > cmd_dequeue_dma = xhci_trb_virt_to_dma(xhci->cmd_ring->deq_seg, > cmd_trb); > /* > > and another for transfer completion: > > @@ -2475,6 +2480,8 @@ static int xhci_handle_event(struct xhci_hcd *xhci) > xhci->event_ring->cycle_state) > return 0; > > + trace_xhci_handle_event(xhci->event_ring, &event->generic); > + > /* > * Barrier between reading the TRB_CYCLE (valid) flag above and any > * speculative reads of the event's flags/data below. > > > I can't see what we're missing here. > Oh, yes. Thanks. Best regards, Lu Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/1] usb: xhci: clear EINT bit in status correctly
EINT(Event Interrupt) is a write-1-to-clear type of bit in xhci status register. It should be cleared by writing a 1. Writing 0 to this bit has no effect. Xhci driver tries to clear this bit by writing 0 to it. This is not the right way to go. This patch corrects this by reading the register first, then clearing all RO/RW1C/RsvZ bits and setting the clearing bit, and writing back the new value at last. Xhci spec requires that software that uses EINT shall clear it prior to clearing any IP flags in section 5.4.2. This is the reason why this patch is CC'ed stable as well. Cc: # v3.14+ Cc: Felipe Balbi Signed-off-by: Lu Baolu --- Change log: v1->v2: - Fixed a warning reported by 0-day kbuild robot. drivers/usb/host/xhci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 1cd5641..bb08c6b 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -721,7 +721,7 @@ void xhci_stop(struct usb_hcd *hcd) xhci_dbg_trace(xhci, trace_xhci_dbg_init, "// Disabling event ring interrupts"); temp = readl(&xhci->op_regs->status); - writel(temp & ~STS_EINT, &xhci->op_regs->status); + writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status); temp = readl(&xhci->ir_set->irq_pending); writel(ER_IRQ_DISABLE(temp), &xhci->ir_set->irq_pending); xhci_print_ir_set(xhci, 0); @@ -1054,7 +1054,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated) xhci_dbg(xhci, "// Disabling event ring interrupts\n"); temp = readl(&xhci->op_regs->status); - writel(temp & ~STS_EINT, &xhci->op_regs->status); + writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status); temp = readl(&xhci->ir_set->irq_pending); writel(ER_IRQ_DISABLE(temp), &xhci->ir_set->irq_pending); xhci_print_ir_set(xhci, 0); -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: xhci: Remove ep_trb from xhci_cleanup_halted_endpoint()
Function argument ep_trb for xhci_cleanup_halted_endpoint() isn't
needed anymore. Cleanup it.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index daa94c3..642a070 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1815,8 +1815,7 @@ struct xhci_segment *trb_in_td(struct xhci_hcd *xhci,
static void xhci_cleanup_halted_endpoint(struct xhci_hcd *xhci,
unsigned int slot_id, unsigned int ep_index,
- unsigned int stream_id,
- struct xhci_td *td, union xhci_trb *ep_trb,
+ unsigned int stream_id, struct xhci_td *td,
enum xhci_ep_reset_type reset_type)
{
struct xhci_virt_ep *ep = &xhci->devs[slot_id]->eps[ep_index];
@@ -1957,8 +1956,7 @@ static int finish_td(struct xhci_hcd *xhci, struct
xhci_td *td,
* The class driver clears the device side halt later.
*/
xhci_cleanup_halted_endpoint(xhci, slot_id, ep_index,
- ep_ring->stream_id, td, ep_trb,
- EP_HARD_RESET);
+ ep_ring->stream_id, td, EP_HARD_RESET);
} else {
/* Update ring dequeue pointer */
while (ep_ring->dequeue != td->last_trb)
@@ -2318,7 +2316,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
case COMP_INVALID_STREAM_TYPE_ERROR:
case COMP_INVALID_STREAM_ID_ERROR:
xhci_cleanup_halted_endpoint(xhci, slot_id, ep_index, 0,
-NULL, NULL, EP_SOFT_RESET);
+NULL, EP_SOFT_RESET);
goto cleanup;
case COMP_RING_UNDERRUN:
case COMP_RING_OVERRUN:
@@ -2584,8 +2582,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
xhci_cleanup_halted_endpoint(xhci, slot_id,
ep_index,
ep_ring->stream_id,
-td, ep_trb,
-EP_HARD_RESET);
+td, EP_HARD_RESET);
goto cleanup;
}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] usb: xhci: Remove ep_trb from finish_td()
Function argument ep_trb for finish_td() isn't needed anymore.
Cleanup it.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 642a070..88071c4 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1921,7 +1921,7 @@ static int xhci_td_cleanup(struct xhci_hcd *xhci, struct
xhci_td *td,
}
static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td,
- union xhci_trb *ep_trb, struct xhci_transfer_event *event,
+ struct xhci_transfer_event *event,
struct xhci_virt_ep *ep, int *status)
{
struct xhci_virt_device *xdev;
@@ -2081,7 +2081,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct
xhci_td *td,
td->urb->actual_length = requested;
finish_td:
- return finish_td(xhci, td, ep_trb, event, ep, status);
+ return finish_td(xhci, td, event, ep, status);
}
/*
@@ -2168,7 +2168,7 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct
xhci_td *td,
td->urb->actual_length += frame->actual_length;
- return finish_td(xhci, td, ep_trb, event, ep, status);
+ return finish_td(xhci, td, event, ep, status);
}
static int skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
@@ -2258,7 +2258,7 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci,
struct xhci_td *td,
remaining);
td->urb->actual_length = 0;
}
- return finish_td(xhci, td, ep_trb, event, ep, status);
+ return finish_td(xhci, td, event, ep, status);
}
/*
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/5] usb: early: add driver for xhci debug capability
Hi, On 05/30/2017 09:46 PM, Vlastimil Babka wrote: > On 03/21/2017 09:01 AM, Lu Baolu wrote: >> XHCI debug capability (DbC) is an optional but standalone >> functionality provided by an xHCI host controller. Software >> learns this capability by walking through the extended >> capability list of the host. XHCI specification describes >> DbC in the section 7.6. >> >> This patch introduces the code to probe and initialize the >> debug capability hardware during early boot. With hardware >> initialized, the debug target (system on which this code is >> running) will present a debug device through the debug port >> (normally the first USB3 port). The debug device is fully >> compliant with the USB framework and provides the equivalent >> of a very high performance (USB3) full-duplex serial link >> between the debug host and target. The DbC functionality is >> independent of the xHCI host. There isn't any precondition >> from the xHCI host side for the DbC to work. >> >> One use for this feature is kernel debugging, for example >> when your machine crashes very early before the regular >> console code is initialized. Other uses include simpler, >> lockless logging instead of a full-blown printk console >> driver and klogd. >> >> Cc: Ingo Molnar >> Cc: Mathias Nyman >> Signed-off-by: Lu Baolu > ... > >> + >> +#define XDBC_TRACE >> +#ifdef XDBC_TRACE >> +#define xdbc_trace trace_printk > Did you forget to remove the #define XDBC_TRACE? > > Enabling this driver brings the "trace_printk() being used. Allocating > extra memory. This means that this is a DEBUG kernel and it is unsafe > for production use." message in 4.12-rcX dmesg. This feature is only for a DEBUG kernel, should not be enabled for any production kernel. This was the reason I enabled trace_printk() by default. Best regards, Lu Baolu > Thanks, > Vlastimil > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/5] usb: early: add driver for xhci debug capability
Hi, On 05/31/2017 02:24 PM, Steven Rostedt wrote: > On Wed, 31 May 2017 11:27:19 +0800 > Lu Baolu wrote: > > >>>> + >>>> +#define XDBC_TRACE >>>> +#ifdef XDBC_TRACE >>>> +#define xdbc_trace trace_printk >>> Did you forget to remove the #define XDBC_TRACE? >>> >>> Enabling this driver brings the "trace_printk() being used. >>> Allocating extra memory. This means that this is a DEBUG kernel and >>> it is unsafe for production use." message in 4.12-rcX dmesg. >> This feature is only for a DEBUG kernel, should not be enabled for >> any production kernel. This was the reason I enabled trace_printk() >> by default. > Yes, it is perfectly fine to use trace_printk() for debug only configs. > But if if you have it there and it is helpful to debug something that > happens in a production system, you may want to look into creating a > real tracepoint for the locations. Yes. Good suggestion. I will try this. Thank you! Best regards, Lu Baolu > > -- Steve > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/5] usb: early: add driver for xhci debug capability
Hi, On 05/31/2017 05:38 PM, Vlastimil Babka wrote: > On 05/31/2017 05:27 AM, Lu Baolu wrote: >> Hi, >> >> On 05/30/2017 09:46 PM, Vlastimil Babka wrote: >>> On 03/21/2017 09:01 AM, Lu Baolu wrote: >>>> XHCI debug capability (DbC) is an optional but standalone >>>> functionality provided by an xHCI host controller. Software >>>> learns this capability by walking through the extended >>>> capability list of the host. XHCI specification describes >>>> DbC in the section 7.6. >>>> >>>> This patch introduces the code to probe and initialize the >>>> debug capability hardware during early boot. With hardware >>>> initialized, the debug target (system on which this code is >>>> running) will present a debug device through the debug port >>>> (normally the first USB3 port). The debug device is fully >>>> compliant with the USB framework and provides the equivalent >>>> of a very high performance (USB3) full-duplex serial link >>>> between the debug host and target. The DbC functionality is >>>> independent of the xHCI host. There isn't any precondition >>>> from the xHCI host side for the DbC to work. >>>> >>>> One use for this feature is kernel debugging, for example >>>> when your machine crashes very early before the regular >>>> console code is initialized. Other uses include simpler, >>>> lockless logging instead of a full-blown printk console >>>> driver and klogd. >>>> >>>> Cc: Ingo Molnar >>>> Cc: Mathias Nyman >>>> Signed-off-by: Lu Baolu >>> ... >>> >>>> + >>>> +#define XDBC_TRACE >>>> +#ifdef XDBC_TRACE >>>> +#define xdbc_trace trace_printk >>> Did you forget to remove the #define XDBC_TRACE? >>> >>> Enabling this driver brings the "trace_printk() being used. Allocating >>> extra memory. This means that this is a DEBUG kernel and it is unsafe >>> for production use." message in 4.12-rcX dmesg. >> This feature is only for a DEBUG kernel, should not be enabled for >> any production kernel. This was the reason I enabled trace_printk() >> by default. > Hmm, but it seems we generally enable all these early printk > features/drivers in our distro kernels. They are not active without a > early_printk=X bootparam anyway, right? It's much more convenient to > e.g. just tell customer to change a param when debugging something than > to install a debug kernel. So I wouldn't agree that only a DEBUG kernel > should have this compiled in. Okay, I will try to find a fix. Best regards, Lu Baolu > > Thanks, > Vlastimil > >> Best regards, >> Lu Baolu >> >>> Thanks, >>> Vlastimil >>> > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] usb/early: Remove trace_printk() callers in xhci-dbc
Trace_printk() was used to log debug messages in xhci-dbc.c where printk() isn't feasible. As there should not be a single caller to trace_printk() in normal kernels, replace them with empty functions. Cc: Vlastimil Babka Cc: Steven Rostedt Cc: Peter Zijlstra Cc: Greg Kroah-Hartman Signed-off-by: Lu Baolu --- drivers/usb/early/xhci-dbc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c index 1268818..12fe70b 100644 --- a/drivers/usb/early/xhci-dbc.c +++ b/drivers/usb/early/xhci-dbc.c @@ -32,7 +32,6 @@ static struct xdbc_state xdbc; static bool early_console_keep; -#define XDBC_TRACE #ifdef XDBC_TRACE #definexdbc_trace trace_printk #else -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 1/5] x86: add simple udelay calibration
Hi,
On 07/12/2017 04:02 PM, Dou Liyang wrote:
> Hi, Lu
>
> At 05/05/2017 08:50 PM, Boris Ostrovsky wrote:
>> On 05/05/2017 01:41 AM, Lu Baolu wrote:
>>> Hi,
>>>
>>> On 05/03/2017 06:38 AM, Boris Ostrovsky wrote:
>>>> On 03/21/2017 04:01 AM, Lu Baolu wrote:
>>>>> Add a simple udelay calibration in x86 architecture-specific
>>>>> boot-time initializations. This will get a workable estimate
>>>>> for loops_per_jiffy. Hence, udelay() could be used after this
>>>>> initialization.
>>>> This breaks Xen PV guests since at this point, and until
>>>> x86_init.paging.pagetable_init() which is when pvclock_vcpu_time_info is
>>>> mapped, they cannot access pvclock.
>>>>
>>>> Is it reasonable to do this before tsc_init() is called? (The failure
>>>> has nothing to do with tsc_init(), really --- it's just that it is
>>>> called late enough that Xen PV guests get properly initialized.) If it
>>>> is, would it be possible to move simple_udelay_calibration() after
>>>> x86_init.paging.pagetable_init()?
>>> This is currently only used for bare metal. How about by-pass it
>>> for Xen PV guests?
>>
>> It is fixed this for Xen PV guests now (in the sense that we don't crash
>> anymore) but my question is still whether this is not too early. Besides
>> tsc_init() (which might not be important here), at the time when
>> simple_udelay_calibration() is invoked we haven't yet called:
>> * kvmclock_init(), which sets calibration routines for KVM
>> * init_hypervisor_platform(), which sets calibration routines for vmware
>> and Xen HVM
>> * x86_init.paging.pagetable_init(), which sets calibration routines for
>> Xen PV
>>
>
> I guess these may have been missed.
>
> Do you have any comments about these?
>
The patch will be available in 4.13-rc1.
Best regards,
Lu Baolu
>> -boris
>>
>>
>>>
>>> Best regards,
>>> Lu Baolu
>>>
>>>> -boris
>>>>
>>>>
>>>>> Cc: Ingo Molnar
>>>>> Cc: x...@kernel.org
>>>>> Signed-off-by: Lu Baolu
>>>>> ---
>>>>> arch/x86/kernel/setup.c | 22 ++
>>>>> 1 file changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>>>>> index 4bf0c89..e70204e 100644
>>>>> --- a/arch/x86/kernel/setup.c
>>>>> +++ b/arch/x86/kernel/setup.c
>>>>> @@ -837,6 +837,26 @@ dump_kernel_offset(struct notifier_block *self,
>>>>> unsigned long v, void *p)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static void __init simple_udelay_calibration(void)
>>>>> +{
>>>>> +unsigned int tsc_khz, cpu_khz;
>>>>> +unsigned long lpj;
>>>>> +
>>>>> +if (!boot_cpu_has(X86_FEATURE_TSC))
>>>>> +return;
>
>
> if it returns here, can we use udelay() correctly like before?
>
> Thanks,
>
> dou.
>
>>>>> +
>>>>> +cpu_khz = x86_platform.calibrate_cpu();
>>>>> +tsc_khz = x86_platform.calibrate_tsc();
>>>>> +
>>>>> +tsc_khz = tsc_khz ? : cpu_khz;
>>>>> +if (!tsc_khz)
>>>>> +return;
>>>>> +
>>>>> +lpj = tsc_khz * 1000;
>>>>> +do_div(lpj, HZ);
>>>>> +loops_per_jiffy = lpj;
>>>>> +}
>>>>> +
>>>>> /*
>>>>> * Determine if we were loaded by an EFI loader. If so, then we have
>>>>> also been
>>>>> * passed the efi memmap, systab, etc., so we should use these data
>>>>> structures
>>>>> @@ -985,6 +1005,8 @@ void __init setup_arch(char **cmdline_p)
>>>>> */
>>>>> x86_configure_nx();
>>>>>
>>>>> +simple_udelay_calibration();
>>>>> +
>>>>> parse_early_param();
>>>>>
>>>>> #ifdef CONFIG_MEMORY_HOTPLUG
>>
>>
>>
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 1/5] x86: add simple udelay calibration
Hi,
On 07/13/2017 09:39 AM, Dou Liyang wrote:
> Hi, Lu
>
> At 07/13/2017 09:17 AM, Lu Baolu wrote:
>> Hi,
>>
>> On 07/12/2017 04:02 PM, Dou Liyang wrote:
>>> Hi, Lu
>>>
>>> At 05/05/2017 08:50 PM, Boris Ostrovsky wrote:
>>>> On 05/05/2017 01:41 AM, Lu Baolu wrote:
>>>>> Hi,
>>>>>
>>>>> On 05/03/2017 06:38 AM, Boris Ostrovsky wrote:
>>>>>> On 03/21/2017 04:01 AM, Lu Baolu wrote:
>>>>>>> Add a simple udelay calibration in x86 architecture-specific
>>>>>>> boot-time initializations. This will get a workable estimate
>>>>>>> for loops_per_jiffy. Hence, udelay() could be used after this
>>>>>>> initialization.
>>>>>> This breaks Xen PV guests since at this point, and until
>>>>>> x86_init.paging.pagetable_init() which is when pvclock_vcpu_time_info is
>>>>>> mapped, they cannot access pvclock.
>>>>>>
>>>>>> Is it reasonable to do this before tsc_init() is called? (The failure
>>>>>> has nothing to do with tsc_init(), really --- it's just that it is
>>>>>> called late enough that Xen PV guests get properly initialized.) If it
>>>>>> is, would it be possible to move simple_udelay_calibration() after
>>>>>> x86_init.paging.pagetable_init()?
>>>>> This is currently only used for bare metal. How about by-pass it
>>>>> for Xen PV guests?
>>>>
>>>> It is fixed this for Xen PV guests now (in the sense that we don't crash
>>>> anymore) but my question is still whether this is not too early. Besides
>>>> tsc_init() (which might not be important here), at the time when
>>>> simple_udelay_calibration() is invoked we haven't yet called:
>>>> * kvmclock_init(), which sets calibration routines for KVM
>>>> * init_hypervisor_platform(), which sets calibration routines for vmware
>>>> and Xen HVM
>>>> * x86_init.paging.pagetable_init(), which sets calibration routines for
>>>> Xen PV
>>>>
>>>
>>> I guess these may have been missed.
>>>
>>> Do you have any comments about these?
>>>
>>
>> The patch will be available in 4.13-rc1.
>
> Yes, I have seen it in the upstream.
>
> Firstly, I also met this problem want to call udelay() earlier than
> *loops_per_jiffy* setup like you[1]. So I am very interesting in this
> patch. ;)
>
> I am also confused about the questions which Boris asked:
>
> whether do the CPU and TSC calibration too early just for using
> udelay()?
>
> this design broke our interface of x86_paltform.calibrate_cpu/tsc.
>
> And I also have a question below.
>
> [...]
>
>>>>>>>
>>>>>>> +static void __init simple_udelay_calibration(void)
>>>>>>> +{
>>>>>>> +unsigned int tsc_khz, cpu_khz;
>>>>>>> +unsigned long lpj;
>>>>>>> +
>>>>>>> +if (!boot_cpu_has(X86_FEATURE_TSC))
>>>>>>> +return;
>
> if we don't have the TSC feature in booting CPU and
> it returns here, can we use udelay() correctly like before?
>
If we have TSC feature, we calculate a preciser loops_per_jiffy here.
Otherwise, we just keep it as before. This function doesn't broke the
use of udelay().
Best regards,
Lu Baolu
>
> [1] https://lkml.org/lkml/2017/7/3/276
>
> Thanks,
>
> dou.
>
>>> Thanks,
>>>
>>> dou.
>>>
>>>>>>> +
>>>>>>> +cpu_khz = x86_platform.calibrate_cpu();
>>>>>>> +tsc_khz = x86_platform.calibrate_tsc();
>>>>>>> +
>>>>>>> +tsc_khz = tsc_khz ? : cpu_khz;
>>>>>>> +if (!tsc_khz)
>>>>>>> +return;
>>>>>>> +
>>>>>>> +lpj = tsc_khz * 1000;
>>>>>>> +do_div(lpj, HZ);
>>>>>>> +loops_per_jiffy = lpj;
>>>>>>> +}
>>>>>>> +
>>>>>>> /*
>>>>>>> * Determine if we were loaded by an EFI loader. If so, then we have
>>>>>>> also been
>>>>>>> * passed the efi memmap, systab, etc., so we should use these data
>>>>>>> structures
>>>>>>> @@ -985,6 +1005,8 @@ void __init setup_arch(char **cmdline_p)
>>>>>>> */
>>>>>>> x86_configure_nx();
>>>>>>>
>>>>>>> +simple_udelay_calibration();
>>>>>>> +
>>>>>>> parse_early_param();
>>>>>>>
>>>>>>> #ifdef CONFIG_MEMORY_HOTPLUG
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>>
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] usb: xhci: Add DbC support in xHCI driver
xHCI compatible USB host controllers(i.e. super-speed USB3 controllers) can be implemented with the Debug Capability(DbC). It presents a debug device which is fully compliant with the USB framework and provides the equivalent of a very high performance full-duplex serial link. The debug capability operation model and registers interface are defined in 7.6.8 of the xHCI specification, revision 1.1. The DbC debug device shares a root port with the xHCI host. By default, the debug capability is disabled and the root port is assigned to xHCI. When the DbC is enabled, the root port will be assigned to the DbC debug device, and the xHCI sees nothing on this port. This implementation uses a sysfs node named under the xHCI device to manage the enabling and disabling of the debug capability. When the debug capability is enabled, it will present a debug device through the debug port. This debug device is fully compliant with the USB3 framework, and it can be enumerated by a debug host on the other end of the USB link. As soon as the debug device is configured, a TTY serial device named /dev/ttyGSn will be created. One use of this link is running a login service on the debug target. Hence it can be remote accessed by a debug host. Another use case can probably be found in servers. It provides a peer-to-peer USB link between two host-only machines. This provides a reasonable out-of-band communication method between two servers. Signed-off-by: Lu Baolu --- .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd | 25 + drivers/usb/host/Kconfig | 10 + drivers/usb/host/Makefile |5 + drivers/usb/host/xhci-dbgcap.c | 1128 drivers/usb/host/xhci-trace.h | 64 ++ drivers/usb/host/xhci.c|9 + drivers/usb/host/xhci.h| 189 +++- 7 files changed, 1429 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd create mode 100644 drivers/usb/host/xhci-dbgcap.c diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd new file mode 100644 index 000..0088aba --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd @@ -0,0 +1,25 @@ +What: /sys/bus/pci/drivers/xhci_hcd/.../dbc +Date: June 2017 +Contact: Lu Baolu +Description: + xHCI compatible USB host controllers (i.e. super-speed + USB3 controllers) are often implemented with the Debug + Capability (DbC). It can present a debug device which + is fully compliant with the USB framework and provides + the equivalent of a very high performance full-duplex + serial link for debug purpose. + + The DbC debug device shares a root port with xHCI host. + When the DbC is enabled, the root port will be assigned + to the Debug Capability. Otherwise, it will be assigned + to xHCI. + + Writing "enable" to this attribute will enable the DbC + functionality and the shared root port will be assigned + to the DbC device. Writing "disable" to this attribute + will disable the DbC functionality and the shared root + port will roll back to the xHCI. + + Reading this attribute gives the state of the DbC. It + can be one of the following states: disabled, enabled, + initialized, connected, configured and stalled. diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index fa5692d..cfb2450 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -27,6 +27,16 @@ config USB_XHCI_HCD module will be called xhci-hcd. if USB_XHCI_HCD +config USB_XHCI_DBGCAP + bool "xHCI support for debug capability" + depends on TTY + depends on USB_GADGET=y + select USB_U_SERIAL + ---help--- + Say 'Y' to enable the support for the xHCI debug capability. Make + sure that your xHCI host supports the extended debug capability and + you want a TTY serial device based on the xHCI debug capability + before enabling this option. If unsure, say 'N'. config USB_XHCI_PCI tristate diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile index cf2691f..4629d20 100644 --- a/drivers/usb/host/Makefile +++ b/drivers/usb/host/Makefile @@ -13,6 +13,11 @@ fhci-$(CONFIG_FHCI_DEBUG) += fhci-dbg.o xhci-hcd-y := xhci.o xhci-mem.o xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o xhci-hcd-y += xhci-trace.o + +ifneq ($(CONFIG_USB_XHCI_DBGCAP), ) + xhci-hcd-y += xhci-dbgcap.o +endif + ifneq ($(CONFIG_USB_XHCI_MTK), ) xhci-hcd-y += xhci-mtk-sch.o endif diff --git a/driv
[PATCH 3/3] usb: doc: Update document for USB3 debug port usage
Update Documentation/driver-api/usb/usb3-debug-port.rst. This update includes the guide for using xHCI debug capability based TTY serial link. Signed-off-by: Lu Baolu --- Documentation/driver-api/usb/usb3-debug-port.rst | 68 1 file changed, 68 insertions(+) diff --git a/Documentation/driver-api/usb/usb3-debug-port.rst b/Documentation/driver-api/usb/usb3-debug-port.rst index feb1a36..e4bb02e 100644 --- a/Documentation/driver-api/usb/usb3-debug-port.rst +++ b/Documentation/driver-api/usb/usb3-debug-port.rst @@ -98,3 +98,71 @@ you to check the sanity of the setup. cat /dev/ttyUSB0 done = end of bash scripts === + +Serial TTY +== + +DbC has also been designed for a serial TTY device at runtime. +One use of this is running a login service on the debug target. +Hence it can be remote accessed by the debug host. Another use +can probably be found in servers. It provides a peer-to-peer USB +link between two host-only machines. This provides a reasonable +out-of-band communication method between two servers. + +In order to use this, you need to make sure your kernel has been +configured to support USB_XHCI_DBGCAP. A sysfs attribute under +the xHCI device node is used to enable or disable DbC. By default, +DbC is disabled:: + + root@target:/sys/bus/pci/devices/:00:14.0# cat dbc + disabled + +Enable DbC with the following command:: + + root@target:/sys/bus/pci/devices/:00:14.0# echo enable > dbc + +You can check the DbC state at anytime:: + + root@target:/sys/bus/pci/devices/:00:14.0# cat dbc + enabled + +Connect the debug target to the debug host with a USB 3.0 super- +speed A-to-A debugging cable. You can see the /dev/ttyGSn created +on the debug target. You will see below kernel message lines:: + + root@target: tail -f /var/log/kern.log + [ 182.730103] xhci_hcd :00:14.0: DbC connected + [ 191.169420] xhci_hcd :00:14.0: DbC configured + [ 191.169597] xhci_hcd :00:14.0: DbC now attached to /dev/ttyGS0 + +Accordingly, the DbC state has been brought up to:: + + root@host:/sys/bus/pci/devices/:00:14.0# cat dbc + configured + +On the debug host, you will see the debug device has been enumerated. +You will see below kernel message lines:: + + root@host: tail -f /var/log/kern.log + [ 79.454780] usb 2-2.1: new SuperSpeed USB device number 3 using xhci_hcd + [ 79.475003] usb 2-2.1: LPM exit latency is zeroed, disabling LPM. + [ 79.475389] usb 2-2.1: New USB device found, idVendor=1d6b, idProduct=0004 + [ 79.475390] usb 2-2.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 + [ 79.475391] usb 2-2.1: Product: Remote GDB + [ 79.475392] usb 2-2.1: Manufacturer: Linux + [ 79.475393] usb 2-2.1: SerialNumber: 0001 + [ 79.660368] usb_debug 2-2.1:1.0: xhci_dbc converter detected + [ 79.660439] usb 2-2.1: xhci_dbc converter now attached to ttyUSB0 + +You can simply verify whether it works by:: + + # On target side + root@target: echo "Hello world" > /dev/ttyGS0 + + # On host side + root@host: cat /dev/ttyUSB0 + Hello world + + # vice versa + +You have a workable serial link over USB now. -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] usb: xhci: Add debug capability support in xhci
Hi, This series is for xHCI debug capability (spec section 7.6.8) support in the xHCI driver. xHCI compatible USB host controllers(i.e. super-speed USB3 controllers) can be implemented with the Debug Capability(DbC). It presents a debug device which is fully compliant with the USB framework and provides the equivalent of a very high performance full-duplex serial link. The debug capability operation model and registers interface are defined in 7.6.8 of the xHCI specification, revision 1.1. The DbC debug device shares a root port with the xHCI host. By default, the debug capability is disabled and the root port is assigned to xHCI. When the DbC is enabled, the root port will be assigned to the DbC debug device, and the xHCI sees nothing on this port. This implementation uses a sysfs node named under the xHCI device to manage the enabling and disabling of the debug capability. When the debug capability is enabled, it will present a debug device through the debug port. This debug device is fully compliant with the USB3 framework, and it can be enumerated by a debug host on the other end of the USB link. As soon as the debug device is configured, a TTY serial device named /dev/ttyGSn will be created. One use of this link is running a login service on the debug target. Hence it can be remote accessed by a debug host. Another use case can probably be found in servers. It provides a peer-to-peer USB link between two host-only machines. This provides a reasonable out-of-band communication method between two servers. Best regards, Lu Baolu Lu Baolu (3): usb: xhci: Make some static functions global usb: xhci: Add DbC support in xHCI driver usb: doc: Update document for USB3 debug port usage .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd | 25 + Documentation/driver-api/usb/usb3-debug-port.rst | 68 ++ drivers/usb/host/Kconfig | 10 + drivers/usb/host/Makefile |5 + drivers/usb/host/xhci-dbgcap.c | 1128 drivers/usb/host/xhci-mem.c| 94 +- drivers/usb/host/xhci-ring.c |4 +- drivers/usb/host/xhci-trace.h | 64 ++ drivers/usb/host/xhci.c|9 + drivers/usb/host/xhci.h| 205 +++- 10 files changed, 1569 insertions(+), 43 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd create mode 100644 drivers/usb/host/xhci-dbgcap.c -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] usb: xhci: Make some static functions global
This patch makes some static functions global to avoid duplications
in different files. These functions can be used in the implementation
of xHCI debug capability. There is no functional change.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-mem.c | 94 ++--
drivers/usb/host/xhci-ring.c | 4 +-
drivers/usb/host/xhci.h | 16 +++-
3 files changed, 72 insertions(+), 42 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 2a82c92..1ccb1c5 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -368,7 +368,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd
*xhci,
* Set the end flag and the cycle toggle bit on the last segment.
* See section 4.9.1 and figures 15 and 16.
*/
-static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
+struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
unsigned int num_segs, unsigned int cycle_state,
enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
{
@@ -467,7 +467,7 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct
xhci_ring *ring,
#define CTX_SIZE(_hcc) (HCC_64BYTE_CONTEXT(_hcc) ? 64 : 32)
-static struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd
*xhci,
+struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd *xhci,
int type, gfp_t flags)
{
struct xhci_container_ctx *ctx;
@@ -492,7 +492,7 @@ static struct xhci_container_ctx
*xhci_alloc_container_ctx(struct xhci_hcd *xhci
return ctx;
}
-static void xhci_free_container_ctx(struct xhci_hcd *xhci,
+void xhci_free_container_ctx(struct xhci_hcd *xhci,
struct xhci_container_ctx *ctx)
{
if (!ctx)
@@ -1762,21 +1762,61 @@ void xhci_free_command(struct xhci_hcd *xhci,
kfree(command);
}
+int xhci_alloc_erst(struct xhci_hcd *xhci,
+ struct xhci_ring *evt_ring,
+ struct xhci_erst *erst,
+ gfp_t flags)
+{
+ size_t size;
+ unsigned int val;
+ struct xhci_segment *seg;
+ struct xhci_erst_entry *entry;
+
+ size = sizeof(struct xhci_erst_entry) * evt_ring->num_segs;
+ erst->entries = dma_alloc_coherent(xhci_to_hcd(xhci)->self.sysdev,
+ size,
+ &erst->erst_dma_addr,
+ flags);
+ if (!erst->entries)
+ return -ENOMEM;
+
+ memset(erst->entries, 0, size);
+ erst->num_entries = evt_ring->num_segs;
+
+ seg = evt_ring->first_seg;
+ for (val = 0; val < evt_ring->num_segs; val++) {
+ entry = &erst->entries[val];
+ entry->seg_addr = cpu_to_le64(seg->dma);
+ entry->seg_size = cpu_to_le32(TRBS_PER_SEGMENT);
+ entry->rsvd = 0;
+ seg = seg->next;
+ }
+
+ return 0;
+}
+
+void xhci_free_erst(struct xhci_hcd *xhci, struct xhci_erst *erst)
+{
+ size_t size;
+ struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
+
+ size = sizeof(struct xhci_erst_entry) * (erst->num_entries);
+ if (erst->entries)
+ dma_free_coherent(dev, size,
+ erst->entries,
+ erst->erst_dma_addr);
+ erst->entries = NULL;
+}
+
void xhci_mem_cleanup(struct xhci_hcd *xhci)
{
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
- int size;
int i, j, num_ports;
cancel_delayed_work_sync(&xhci->cmd_timer);
- /* Free the Event Ring Segment Table and the actual Event Ring */
- size = sizeof(struct xhci_erst_entry)*(xhci->erst.num_entries);
- if (xhci->erst.entries)
- dma_free_coherent(dev, size,
- xhci->erst.entries, xhci->erst.erst_dma_addr);
- xhci->erst.entries = NULL;
- xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed ERST");
+ xhci_free_erst(xhci, &xhci->erst);
+
if (xhci->event_ring)
xhci_ring_free(xhci, xhci->event_ring);
xhci->event_ring = NULL;
@@ -2313,9 +2353,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
unsigned intval, val2;
u64 val_64;
- struct xhci_segment *seg;
- u32 page_size, temp;
- int i;
+ u32 page_size, temp;
+ int i, ret;
INIT_LIST_HEAD(&xhci->cmd_list);
@@ -2454,32 +2493,9 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
if (xhci_check_trb_in_td_math(xhci) < 0)
goto fail;
- xhci->erst.entries =
Re: [PATCH 2/3] usb: xhci: Add DbC support in xHCI driver
Hi Felipe,
On 07/21/2017 06:31 PM, Felipe Balbi wrote:
> Hi,
>
> Lu Baolu writes:
>> +static void xhci_dbc_stop(struct xhci_hcd *xhci)
>> +{
>> +struct xhci_dbc *dbc = xhci->dbc;
>> +
>> +WARN_ON(!dbc);
>> +
>> +cancel_delayed_work_sync(&dbc->event_work);
>> +
>> +if (dbc->gs_port_num != GSPORT_INVAL) {
>> +gserial_disconnect(&dbc->gs_port);
>> +gserial_free_line(dbc->gs_port_num);
> why are you tying host stack to the gadget framework?
XHCI debug capability is actually a debug device gadget.
The hardware and firmware do everything of gadget work
and leave the interface to xHCI for enabling/disabling and
queuing transfer requests.
u_serial.c provides a generic layer between a USB gadget
and the TTY layer. I used it to avoid duplicating code.
>
> With this, you're forcing every single PC in the world to compile the
> gadget framework, that's a bit much don't you think?
>
Yes, you are right. Is it acceptable if I move u_serial.c from
the current place to drivers/usb/common?
Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] usb: xhci: Handle USB transaction error on address command
Xhci driver handles USB transaction errors on transfer events, but transaction errors are possible on address device command completion events as well. The xHCI specification (section 4.6.5) says: A USB Transaction Error Completion Code for an Address Device Command may be due to a Stall response from a device. Software should issue a Disable Slot Command for the Device Slot then an Enable Slot Command to recover from this error. This patch handles USB transaction errors on address command completion events. The related discussion threads can be found through below links. http://marc.info/?l=linux-usb&m=149362010728921&w=2 http://marc.info/?l=linux-usb&m=149252752825755&w=2 Suggested-by: Mathias Nyman Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index b2ff1ff..9cc56cd 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3836,6 +3836,12 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, ret = -EINVAL; break; case COMP_USB_TRANSACTION_ERROR: + xhci_free_virt_device(xhci, udev->slot_id); + ret = xhci_disable_slot(xhci, command, udev->slot_id); + udev->slot_id = 0; + if (!ret) + xhci_alloc_dev(hcd, udev); + dev_warn(&udev->dev, "Device not responding to setup %s.\n", act); ret = -EPROTO; break; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] usb: xhci: Handle USB transaction error on address command
Hi, On 07/26/2017 01:11 PM, Xing, Zhengjun wrote: > > > On 7/25/2017 1:09 PM, Lu Baolu wrote: >> Xhci driver handles USB transaction errors on transfer events, >> but transaction errors are possible on address device command >> completion events as well. >> >> The xHCI specification (section 4.6.5) says: A USB Transaction >> Error Completion Code for an Address Device Command may be due >> to a Stall response from a device. Software should issue a Disable >> Slot Command for the Device Slot then an Enable Slot Command to >> recover from this error. >> >> This patch handles USB transaction errors on address command >> completion events. The related discussion threads can be found >> through below links. >> >> http://marc.info/?l=linux-usb&m=149362010728921&w=2 >> http://marc.info/?l=linux-usb&m=149252752825755&w=2 >> >> Suggested-by: Mathias Nyman >> Signed-off-by: Lu Baolu >> --- >> drivers/usb/host/xhci.c | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >> index b2ff1ff..9cc56cd 100644 >> --- a/drivers/usb/host/xhci.c >> +++ b/drivers/usb/host/xhci.c >> @@ -3836,6 +3836,12 @@ static int xhci_setup_device(struct usb_hcd *hcd, >> struct usb_device *udev, >> ret = -EINVAL; >> break; >> case COMP_USB_TRANSACTION_ERROR: >> +xhci_free_virt_device(xhci, udev->slot_id); > > In xhci_free_virt_device xhci->devs[slot_id]will be set to NULL. > >> +ret = xhci_disable_slot(xhci, command, udev->slot_id); > > When xhci_disable_slot check xhci->devs[slot_id] is NULL, just return > -EINVAL, the slot will not be disabled. Yes, really. I don't think xhci_disable_slot() should return directly when the virtual device structure is not allocated. This function is also used to issue a disable slot command when the virtual device data allocation fails in xhci_alloc_dev(). I will develop v2 patch with a fix for xhci_disable_slot(). Best regards, Lu Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/5] usb: xhci: Fix potential memory leak in xhci_disable_slot()
xhci_disable_slot() allows the invoker to pass a command pointer
as paramenter. Otherwise, it will allocate one. This will cause
memory leak when a command structure was allocated inside of this
function while queuing command trb fails. Another problem comes up
when the invoker passed a command pointer, but xhci_disable_slot()
frees it when it detects a dead host.
This patch fixes these two problems by removing the command parameter
from xhci_disable_slot().
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-hub.c | 2 +-
drivers/usb/host/xhci.c | 30 +-
drivers/usb/host/xhci.h | 3 +--
3 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 00721e8..c862d53 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -612,7 +612,7 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
xhci_dbg(xhci, "Disable all slots\n");
spin_unlock_irqrestore(&xhci->lock, *flags);
for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
- retval = xhci_disable_slot(xhci, NULL, i);
+ retval = xhci_disable_slot(xhci, i);
if (retval)
xhci_err(xhci, "Failed to disable slot %d, %d. Enter
test mode anyway\n",
i, retval);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e69073f..cb2461a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3519,11 +3519,6 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
struct xhci_virt_device *virt_dev;
struct xhci_slot_ctx *slot_ctx;
int i, ret;
- struct xhci_command *command;
-
- command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
- if (!command)
- return;
#ifndef CONFIG_USB_DEFAULT_PERSIST
/*
@@ -3539,10 +3534,8 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
/* If the host is halted due to driver unload, we still need to free the
* device.
*/
- if (ret <= 0 && ret != -ENODEV) {
- kfree(command);
+ if (ret <= 0 && ret != -ENODEV)
return;
- }
virt_dev = xhci->devs[udev->slot_id];
slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx);
@@ -3554,22 +3547,21 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
del_timer_sync(&virt_dev->eps[i].stop_cmd_timer);
}
- xhci_disable_slot(xhci, command, udev->slot_id);
+ xhci_disable_slot(xhci, udev->slot_id);
/*
* Event command completion handler will free any data structures
* associated with the slot. XXX Can free sleep?
*/
}
-int xhci_disable_slot(struct xhci_hcd *xhci, struct xhci_command *command,
- u32 slot_id)
+int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
{
+ struct xhci_command *command;
unsigned long flags;
u32 state;
int ret = 0;
- if (!command)
- command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
+ command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
if (!command)
return -ENOMEM;
@@ -3588,7 +3580,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct
xhci_command *command,
slot_id);
if (ret) {
spin_unlock_irqrestore(&xhci->lock, flags);
- xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
+ kfree(command);
return ret;
}
xhci_ring_cmd_db(xhci);
@@ -3663,6 +3655,8 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device
*udev)
return 0;
}
+ xhci_free_command(xhci, command);
+
if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK)) {
spin_lock_irqsave(&xhci->lock, flags);
ret = xhci_reserve_host_control_ep_resources(xhci);
@@ -3698,18 +3692,12 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct
usb_device *udev)
pm_runtime_get_noresume(hcd->self.controller);
#endif
-
- xhci_free_command(xhci, command);
/* Is this a LS or FS device under a HS hub? */
/* Hub or peripherial? */
return 1;
disable_slot:
- /* Disable slot, if we can do it without mem alloc */
- kfree(command->completion);
- command->completion = NULL;
- command->status = 0;
- return xhci_disable_slot(xhci, command, udev->slot_id);
+ return xhci_disable_slot(xhci, udev->slot_id);
}
/*
diff --git a/drivers/usb/host/xhci.h b/dri
[PATCH v2 4/5] usb: xhci: Return error when host is dead in xhci_disable_slot()
xhci_disable_slot() is a helper for disabling a slot when a device
goes away or recovers from error situations. Currently, it returns
success when it sees a dead host. This is not the right way to go.
It should return error and let the invoker know that disable slot
command was failed due to a dead host.
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 2df601e..d6b728d 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3568,10 +3568,9 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
state = readl(&xhci->op_regs->status);
if (state == 0x || (xhci->xhc_state & XHCI_STATE_DYING) ||
(xhci->xhc_state & XHCI_STATE_HALTED)) {
- xhci_free_virt_device(xhci, slot_id);
spin_unlock_irqrestore(&xhci->lock, flags);
kfree(command);
- return ret;
+ return -ENODEV;
}
ret = xhci_queue_slot_control(xhci, command, TRB_DISABLE_SLOT,
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/5] usb: xhci: Disable slot even virt-dev is null
xhci_disable_slot() is a helper for disabling a slot when a device
goes away or recovers from error situations. Currently, it checks
the corespoding virt-dev pointer and returns directly (w/o issuing
disable slot command) if it's null.
This is unnecessary and will cause problems in case where virt-dev
allocation fails and xhci_disable_slot() is called to roll back the
hardware state. Refer to the implementation of xhci_alloc_dev().
This patch removes lines to check virt-dev in xhci_disable_slot().
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b2ff1ff..e69073f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3567,11 +3567,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct
xhci_command *command,
unsigned long flags;
u32 state;
int ret = 0;
- struct xhci_virt_device *virt_dev;
- virt_dev = xhci->devs[slot_id];
- if (!virt_dev)
- return -EINVAL;
if (!command)
command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
if (!command)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/5] usb: xhci: Handle USB transaction error on address command
Xhci driver handles USB transaction errors on transfer events, but transaction errors are possible on address device command completion events as well. The xHCI specification (section 4.6.5) says: A USB Transaction Error Completion Code for an Address Device Command may be due to a Stall response from a device. Software should issue a Disable Slot Command for the Device Slot then an Enable Slot Command to recover from this error. This patch handles USB transaction errors on address command completion events. The related discussion threads can be found through below links. http://marc.info/?l=linux-usb&m=149362010728921&w=2 http://marc.info/?l=linux-usb&m=149252752825755&w=2 Suggested-by: Mathias Nyman Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index d6b728d..95780f8 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3822,6 +3822,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, break; case COMP_USB_TRANSACTION_ERROR: dev_warn(&udev->dev, "Device not responding to setup %s.\n", act); + + ret = xhci_disable_slot(xhci, udev->slot_id); + if (!ret) + xhci_alloc_dev(hcd, udev); + ret = -EPROTO; break; case COMP_INCOMPATIBLE_DEVICE_ERROR: -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/5] usb: xhci: Fix memory leak when xhci_disable_slot() returns error
If xhci_disable_slot() returns success, a disable slot command trb was queued in the command ring. The command completion handler will free the virtual device data structure associated with the slot. On the other hand, when xhci_disable_slot() returns error, the invokers should take the responsibilities to free the slot related data structure. Otherwise, memory leakage happens. Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index cb2461a..2df601e 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3547,11 +3547,9 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) del_timer_sync(&virt_dev->eps[i].stop_cmd_timer); } - xhci_disable_slot(xhci, udev->slot_id); - /* -* Event command completion handler will free any data structures -* associated with the slot. XXX Can free sleep? -*/ + ret = xhci_disable_slot(xhci, udev->slot_id); + if (ret) + xhci_free_virt_device(xhci, udev->slot_id); } int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id) @@ -3697,7 +3695,11 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) return 1; disable_slot: - return xhci_disable_slot(xhci, udev->slot_id); + ret = xhci_disable_slot(xhci, udev->slot_id); + if (ret) + xhci_free_virt_device(xhci, udev->slot_id); + + return 0; } /* -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/5] usb: xhci: Handle USB transaction error on address command
Xhci driver handles USB transaction errors on transfer events, but transaction errors are possible on address device command completion events as well. The xHCI specification (section 4.6.5) says: A USB Transaction Error Completion Code for an Address Device Command may be due to a Stall response from a device. Software should issue a Disable Slot Command for the Device Slot then an Enable Slot Command to recover from this error. The related discussion threads can be found through below links. http://marc.info/?l=linux-usb&m=149362010728921&w=2 http://marc.info/?l=linux-usb&m=149252752825755&w=2 This patch set includes some fixes in xhci_disable_slot() as well which will be used to handle USB transaction error on address command. --- Change log: v1->v2: - include 4 fixes in xhci_disable_slot which will be used to handle USB transaction error on address command. Lu Baolu (5): usb: xhci: Disable slot even virt-dev is null usb: xhci: Fix potential memory leak in xhci_disable_slot() usb: xhci: Fix memory leak when xhci_disable_slot() returns error usb: xhci: Return error when host is dead in xhci_disable_slot() usb: xhci: Handle USB transaction error on address command drivers/usb/host/xhci-hub.c | 2 +- drivers/usb/host/xhci.c | 52 ++--- drivers/usb/host/xhci.h | 3 +-- 3 files changed, 23 insertions(+), 34 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] usb: xhci: Handle USB transaction error on address command
Hi, On 07/27/2017 03:55 PM, Felipe Balbi wrote: > Hi, > > Lu Baolu writes: >> Xhci driver handles USB transaction errors on transfer events, >> but transaction errors are possible on address device command >> completion events as well. >> >> The xHCI specification (section 4.6.5) says: A USB Transaction >> Error Completion Code for an Address Device Command may be due >> to a Stall response from a device. Software should issue a Disable >> Slot Command for the Device Slot then an Enable Slot Command to >> recover from this error. >> >> This patch handles USB transaction errors on address command >> completion events. The related discussion threads can be found >> through below links. >> >> http://marc.info/?l=linux-usb&m=149362010728921&w=2 >> http://marc.info/?l=linux-usb&m=149252752825755&w=2 >> >> Suggested-by: Mathias Nyman >> Signed-off-by: Lu Baolu >> --- >> drivers/usb/host/xhci.c | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >> index d6b728d..95780f8 100644 >> --- a/drivers/usb/host/xhci.c >> +++ b/drivers/usb/host/xhci.c >> @@ -3822,6 +3822,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, >> struct usb_device *udev, >> break; >> case COMP_USB_TRANSACTION_ERROR: >> dev_warn(&udev->dev, "Device not responding to setup %s.\n", >> act); >> + >> +ret = xhci_disable_slot(xhci, udev->slot_id); >> + if (!ret) >> +xhci_alloc_dev(hcd, udev); > aren't you leaking previously allocated virt_dev ? > When disable slot command completes, the command completion handler will release the previous allocated virt_dev. Best regards, Lu Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] usb: xhci: Add debugfs interface for xHCI driver
This adds debugfs consumer for xHCI driver. The debugfs entries
read all host registers, device/endpoint contexts, command ring,
event ring and various endpoint rings. With these entries, users
can check the registers and memory spaces used by a host during
run time, or save all the information with a simple 'cp -r' for
post-mortem programs.
The file hierarchy looks like this.
[root of debugfs]
|__usb
|[e,u,o]hci <-[root for other HCIs]
|xhci <---[root for xHCI]
|__:00:14.0 <--[xHCI host name]
|reg-cap<[capability registers]
|reg-op <---[operational registers]
|reg-runtime<---[runtime registers]
|reg-ext-#cap_name <[extended capability regs]
|command-ring <---[root for command ring]
|__cycle<--[ring cycle]
|__dequeue <[ring dequeue pointer]
|__enqueue <[ring enqueue pointer]
|__trbs <---[ring trbs]
|event-ring <-[root for event ring]
|__cycle<--[ring cycle]
|__dequeue <[ring dequeue pointer]
|__enqueue <[ring enqueue pointer]
|__trbs <---[ring trbs]
|devices<[root for devices]
|__#slot_id <---[root for a device]
|name <-[device name]
|slot-context <[slot context]
|ep-context <---[endpoint contexts]
|ep#ep_index<[root for an endpoint]
|__cycle<--[ring cycle]
|__dequeue <[ring dequeue pointer]
|__enqueue <[ring enqueue pointer]
|__trbs <---[ring trbs]
Signed-off-by: Lu Baolu
---
drivers/usb/host/Makefile | 4 +
drivers/usb/host/xhci-debugfs.c | 552
drivers/usb/host/xhci-debugfs.h | 137 ++
drivers/usb/host/xhci-mem.c | 4 +-
drivers/usb/host/xhci.c | 21 +-
drivers/usb/host/xhci.h | 9 +
6 files changed, 723 insertions(+), 4 deletions(-)
create mode 100644 drivers/usb/host/xhci-debugfs.c
create mode 100644 drivers/usb/host/xhci-debugfs.h
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index cf2691f..b2a7f05 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -25,6 +25,10 @@ ifneq ($(CONFIG_USB_XHCI_RCAR), )
xhci-plat-hcd-y += xhci-rcar.o
endif
+ifneq ($(CONFIG_DEBUG_FS),)
+ xhci-hcd-y += xhci-debugfs.o
+endif
+
obj-$(CONFIG_USB_WHCI_HCD) += whci/
obj-$(CONFIG_USB_PCI) += pci-quirks.o
diff --git a/drivers/usb/host/xhci-debugfs.c b/drivers/usb/host/xhci-debugfs.c
new file mode 100644
index 000..7588ac6
--- /dev/null
+++ b/drivers/usb/host/xhci-debugfs.c
@@ -0,0 +1,552 @@
+/*
+ * xhci-debugfs.c - xHCI debugfs interface
+ *
+ * Copyright (C) 2017 Intel Corporation
+ *
+ * Author: Lu Baolu
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+
+#include "xhci.h"
+#include "xhci-debugfs.h"
+
+static const struct debugfs_reg32 xhci_cap_regs[] = {
+ dump_register(CAPLENGTH),
+ dump_register(HCSPARAMS1),
+ dump_register(HCSPARAMS2),
+ dump_register(HCSPARAMS3),
+ dump_register(HCCPARAMS1),
+ dump_register(DOORBELLOFF),
+ dump_register(RUNTIMEOFF),
+ dump_register(HCCPARAMS2),
+};
+
+static const struct debugfs_reg32 xhci_op_regs[] = {
+ dump_register(USBCMD),
+ dump_register(USBSTS),
+ dump_register(PAGESIZE),
+ dump_register(DNCTRL),
+ dump_register(CRCR),
+ dump_register(DCBAAP_LOW),
+ dump_register(DCBAAP_HIGH),
+ dump_register(CONFIG),
+};
+
+static const struct debugfs_reg32 xhci_runtime_regs[] = {
+ dump_register(MFINDEX),
+ dump_register(IR0_IMAN),
+ dump_register(IR0_IMOD),
+ dump_register(IR0_ERSTSZ),
+ dump_register(IR0_ERSTBA_LOW),
+ dump_register(IR0_ERSTBA_HIGH),
+ dump_register(IR0_ERDP_LOW),
+ dump_register(IR0_ERDP_HIGH),
+};
+
+static const struct debugfs_reg32 xhci_extcap_legsup[] = {
+ dump_register(EXTCAP_USBLEGSUP),
+ dump_register(EXTCAP_USBLEGCTLSTS),
+};
+
+static const struct debugfs_reg32 xhci_extcap_protocol[] = {
+ dump_register(EXTC
Re: [PATCH 1/1] usb: xhci: Add debugfs interface for xHCI driver
Hi Greg,
On 07/29/2017 09:34 PM, Greg KH wrote:
> On Sat, Jul 29, 2017 at 04:18:03PM +0800, Lu Baolu wrote:
>> +static void xhci_debugfs_create_files(struct xhci_hcd *xhci,
>> + struct xhci_file_map *files,
>> + size_t nentries, void *data,
>> + struct dentry *parent,
>> + const struct file_operations *fops)
>> +{
>> +int i;
>> +struct dentry *file;
>> +
>> +for (i = 0; i < nentries; i++) {
>> +file = debugfs_create_file(files[i].name, 0444,
>> + parent, data, fops);
>> +if (IS_ERR_OR_NULL(file))
>> +break;
> There's no need to ever check the return value of a debugfs_ function,
> there's nothing you can do here, just keep calling it :)
>
> And you will not get an error, you will only get NULL if there is an
> error, as the only error you would get is if debugfs was not enabled.
>
>> +static struct dentry *xhci_debugfs_create_ring_dir(struct xhci_hcd *xhci,
>> + struct xhci_ring *ring,
>> + const char *name,
>> + struct dentry *parent)
>> +{
>> +struct dentry *dir;
>> +
>> +dir = debugfs_create_dir(name, parent);
>> +if (IS_ERR_OR_NULL(dir))
>> +return NULL;
> Same here. Just keep going, you don't care about the return value, but
> you can use it safely no matter what.
>
>
> Same for other places in this patch as well.
Thanks for review. I will correct them in a v2 patch.
Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] usb: xhci: Free the right ring in xhci_add_endpoint()
In the xhci_add_endpoint(), a new ring was allocated and saved at
xhci_virt_ep->new_ring. Hence, when error happens, we need to free
the allocated ring before returning error.
Current code frees xhci_virt_ep->ring instead of the new_ring. This
patch fixes this.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b2ff1ff..ee198ea 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1703,7 +1703,8 @@ static int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
if (xhci->quirks & XHCI_MTK_HOST) {
ret = xhci_mtk_add_ep_quirk(hcd, udev, ep);
if (ret < 0) {
- xhci_free_endpoint_ring(xhci, virt_dev, ep_index);
+ xhci_ring_free(xhci, virt_dev->eps[ep_index].new_ring);
+ virt_dev->eps[ep_index].new_ring = NULL;
return ret;
}
}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/1] usb: xhci: Add debugfs interface for xHCI driver
This adds debugfs consumer for xHCI driver. The debugfs entries
read all host registers, device/endpoint contexts, command ring,
event ring and various endpoint rings. With these entries, users
can check the registers and memory spaces used by a host during
run time, or save all the information with a simple 'cp -r' for
post-mortem programs.
The file hierarchy looks like this.
[root of debugfs]
|__usb
|[e,u,o]hci <-[root for other HCIs]
|xhci <---[root for xHCI]
|__:00:14.0 <--[xHCI host name]
|reg-cap<[capability registers]
|reg-op <---[operational registers]
|reg-runtime<---[runtime registers]
|reg-ext-#cap_name <[extended capability regs]
|command-ring <---[root for command ring]
|__cycle<--[ring cycle]
|__dequeue <[ring dequeue pointer]
|__enqueue <[ring enqueue pointer]
|__trbs <---[ring trbs]
|event-ring <-[root for event ring]
|__cycle<--[ring cycle]
|__dequeue <[ring dequeue pointer]
|__enqueue <[ring enqueue pointer]
|__trbs <---[ring trbs]
|devices<[root for devices]
|__#slot_id <---[root for a device]
|name <-[device name]
|slot-context <[slot context]
|ep-context <---[endpoint contexts]
|ep#ep_index<[root for an endpoint]
|__cycle<--[ring cycle]
|__dequeue <[ring dequeue pointer]
|__enqueue <[ring enqueue pointer]
|__trbs <---[ring trbs]
Signed-off-by: Lu Baolu
---
Change log:
v1->v2:
- No need to check return valuse of debugfs interfaces.
- Remove file entries when adding endpoint failed.
drivers/usb/host/Makefile | 4 +
drivers/usb/host/xhci-debugfs.c | 526
drivers/usb/host/xhci-debugfs.h | 137 +++
drivers/usb/host/xhci-mem.c | 5 +-
drivers/usb/host/xhci.c | 23 +-
drivers/usb/host/xhci.h | 9 +
6 files changed, 700 insertions(+), 4 deletions(-)
create mode 100644 drivers/usb/host/xhci-debugfs.c
create mode 100644 drivers/usb/host/xhci-debugfs.h
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index cf2691f..b2a7f05 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -25,6 +25,10 @@ ifneq ($(CONFIG_USB_XHCI_RCAR), )
xhci-plat-hcd-y += xhci-rcar.o
endif
+ifneq ($(CONFIG_DEBUG_FS),)
+ xhci-hcd-y += xhci-debugfs.o
+endif
+
obj-$(CONFIG_USB_WHCI_HCD) += whci/
obj-$(CONFIG_USB_PCI) += pci-quirks.o
diff --git a/drivers/usb/host/xhci-debugfs.c b/drivers/usb/host/xhci-debugfs.c
new file mode 100644
index 000..6772ee9
--- /dev/null
+++ b/drivers/usb/host/xhci-debugfs.c
@@ -0,0 +1,526 @@
+/*
+ * xhci-debugfs.c - xHCI debugfs interface
+ *
+ * Copyright (C) 2017 Intel Corporation
+ *
+ * Author: Lu Baolu
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+
+#include "xhci.h"
+#include "xhci-debugfs.h"
+
+static const struct debugfs_reg32 xhci_cap_regs[] = {
+ dump_register(CAPLENGTH),
+ dump_register(HCSPARAMS1),
+ dump_register(HCSPARAMS2),
+ dump_register(HCSPARAMS3),
+ dump_register(HCCPARAMS1),
+ dump_register(DOORBELLOFF),
+ dump_register(RUNTIMEOFF),
+ dump_register(HCCPARAMS2),
+};
+
+static const struct debugfs_reg32 xhci_op_regs[] = {
+ dump_register(USBCMD),
+ dump_register(USBSTS),
+ dump_register(PAGESIZE),
+ dump_register(DNCTRL),
+ dump_register(CRCR),
+ dump_register(DCBAAP_LOW),
+ dump_register(DCBAAP_HIGH),
+ dump_register(CONFIG),
+};
+
+static const struct debugfs_reg32 xhci_runtime_regs[] = {
+ dump_register(MFINDEX),
+ dump_register(IR0_IMAN),
+ dump_register(IR0_IMOD),
+ dump_register(IR0_ERSTSZ),
+ dump_register(IR0_ERSTBA_LOW),
+ dump_register(IR0_ERSTBA_HIGH),
+ dump_register(IR0_ERDP_LOW),
+ dump_register(IR0_ERDP_HIGH),
+};
+
+static const struct debugfs_reg32 xhci_extcap_legsup[] = {
+ dump_register(EXTCAP_USB
[PATCH v2 2/4] usb: common: Move u_serial from gadget/function to usb/common
The component u_serial provides a glue layer between TTY layer
and a USB gadget device needed to provide a basic serial port
functionality. Currently, u_serial sits under gadget/function
and depends on CONFIG_USB_GADGET to be compiled and used.
Most of the serial gadget devices are based on a UDC (USB device
controller) and implemented by making use of the Linux gadget
frameworks. But we are facing other implementions as well. One
example can be found with xHCI debug capability. The xHCI debug
capability implements a serial gadget with hardware and firmware,
and provides an interface similar with xHCI host for submitting
and reaping the transfer requests.
In order to make better use of u_serial when implementing xHCI
debug capability in xHCI driver, this patch moves u_serial.c
from gadget/function to usb/common, and moves u_serial.h from
gadget/function to include/linux/usb.
Signed-off-by: Lu Baolu
---
drivers/usb/Kconfig|3 +
drivers/usb/Makefile |3 +-
drivers/usb/common/Makefile|1 +
drivers/usb/common/u_serial.c | 1604 +++
drivers/usb/gadget/Kconfig |3 -
drivers/usb/gadget/function/Makefile |1 -
drivers/usb/gadget/function/f_acm.c|4 +-
drivers/usb/gadget/function/f_obex.c |4 +-
drivers/usb/gadget/function/f_serial.c |3 +-
drivers/usb/gadget/function/u_serial.c | 1606
drivers/usb/gadget/function/u_serial.h | 71 --
drivers/usb/gadget/legacy/acm_ms.c |3 +-
drivers/usb/gadget/legacy/cdc2.c |2 +-
drivers/usb/gadget/legacy/dbgp.c |3 +-
drivers/usb/gadget/legacy/multi.c |2 +-
drivers/usb/gadget/legacy/nokia.c |2 +-
drivers/usb/gadget/legacy/serial.c |3 +-
include/linux/usb/u_serial.h | 71 ++
18 files changed, 1689 insertions(+), 1700 deletions(-)
create mode 100644 drivers/usb/common/u_serial.c
delete mode 100644 drivers/usb/gadget/function/u_serial.c
delete mode 100644 drivers/usb/gadget/function/u_serial.h
create mode 100644 include/linux/usb/u_serial.h
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 939a63b..c39aceb 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -35,6 +35,9 @@ config USB_COMMON
config USB_ARCH_HAS_HCD
def_bool y
+config USB_U_SERIAL
+ tristate
+
config USB
tristate "Support for Host-side USB"
depends on USB_ARCH_HAS_HCD
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 9650b35..a3274aa 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -5,6 +5,7 @@
# Object files in subdirectories
obj-$(CONFIG_USB) += core/
+obj-$(CONFIG_USB_COMMON) += common/
obj-$(CONFIG_USB_SUPPORT) += phy/
obj-$(CONFIG_USB_DWC3) += dwc3/
@@ -59,8 +60,6 @@ obj-$(CONFIG_USB_CHIPIDEA)+= chipidea/
obj-$(CONFIG_USB_RENESAS_USBHS)+= renesas_usbhs/
obj-$(CONFIG_USB_GADGET) += gadget/
-obj-$(CONFIG_USB_COMMON) += common/
-
obj-$(CONFIG_USBIP_CORE) += usbip/
obj-$(CONFIG_TYPEC)+= typec/
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
index 6bbb3ec..dff720b 100644
--- a/drivers/usb/common/Makefile
+++ b/drivers/usb/common/Makefile
@@ -8,3 +8,4 @@ usb-common-$(CONFIG_USB_LED_TRIG) += led.o
obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
obj-$(CONFIG_USB_ULPI_BUS) += ulpi.o
+obj-$(CONFIG_USB_U_SERIAL) += u_serial.o
diff --git a/drivers/usb/common/u_serial.c b/drivers/usb/common/u_serial.c
new file mode 100644
index 000..da09602
--- /dev/null
+++ b/drivers/usb/common/u_serial.c
@@ -0,0 +1,1604 @@
+/*
+ * u_serial.c - utilities for USB gadget "serial port"/TTY support
+ *
+ * Copyright (C) 2003 Al Borchers (alborch...@steinerpoint.com)
+ * Copyright (C) 2008 David Brownell
+ * Copyright (C) 2008 by Nokia Corporation
+ *
+ * This code also borrows from usbserial.c, which is
+ * Copyright (C) 1999 - 2002 Greg Kroah-Hartman (g...@kroah.com)
+ * Copyright (C) 2000 Peter Berger (pber...@brimson.com)
+ * Copyright (C) 2000 Al Borchers (alborch...@steinerpoint.com)
+ *
+ * This software is distributed under the terms of the GNU General
+ * Public License ("GPL") as published by the Free Software Foundation,
+ * either version 2 of that License or (at your option) any later version.
+ */
+
+/* #define VERBOSE_DEBUG */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/*
+ * This component encapsulates the TTY layer glue needed to provide basic
+ * "serial port" functionality through the USB gadget stack. Each such
+ * port is exposed through a /dev/ttyGS* node.
+ *
+ * After this module has been loaded, the individual TTY port can be requested
+ * (gserial_alloc_line()) and it will stay available until they are removed
+ * (gserial_fr
[PATCH v2 4/4] usb: doc: Update document for USB3 debug port usage
Update Documentation/driver-api/usb/usb3-debug-port.rst. This update includes the guide for using xHCI debug capability based TTY serial link. Signed-off-by: Lu Baolu --- Documentation/driver-api/usb/usb3-debug-port.rst | 68 1 file changed, 68 insertions(+) diff --git a/Documentation/driver-api/usb/usb3-debug-port.rst b/Documentation/driver-api/usb/usb3-debug-port.rst index feb1a36..e4bb02e 100644 --- a/Documentation/driver-api/usb/usb3-debug-port.rst +++ b/Documentation/driver-api/usb/usb3-debug-port.rst @@ -98,3 +98,71 @@ you to check the sanity of the setup. cat /dev/ttyUSB0 done = end of bash scripts === + +Serial TTY +== + +DbC has also been designed for a serial TTY device at runtime. +One use of this is running a login service on the debug target. +Hence it can be remote accessed by the debug host. Another use +can probably be found in servers. It provides a peer-to-peer USB +link between two host-only machines. This provides a reasonable +out-of-band communication method between two servers. + +In order to use this, you need to make sure your kernel has been +configured to support USB_XHCI_DBGCAP. A sysfs attribute under +the xHCI device node is used to enable or disable DbC. By default, +DbC is disabled:: + + root@target:/sys/bus/pci/devices/:00:14.0# cat dbc + disabled + +Enable DbC with the following command:: + + root@target:/sys/bus/pci/devices/:00:14.0# echo enable > dbc + +You can check the DbC state at anytime:: + + root@target:/sys/bus/pci/devices/:00:14.0# cat dbc + enabled + +Connect the debug target to the debug host with a USB 3.0 super- +speed A-to-A debugging cable. You can see the /dev/ttyGSn created +on the debug target. You will see below kernel message lines:: + + root@target: tail -f /var/log/kern.log + [ 182.730103] xhci_hcd :00:14.0: DbC connected + [ 191.169420] xhci_hcd :00:14.0: DbC configured + [ 191.169597] xhci_hcd :00:14.0: DbC now attached to /dev/ttyGS0 + +Accordingly, the DbC state has been brought up to:: + + root@host:/sys/bus/pci/devices/:00:14.0# cat dbc + configured + +On the debug host, you will see the debug device has been enumerated. +You will see below kernel message lines:: + + root@host: tail -f /var/log/kern.log + [ 79.454780] usb 2-2.1: new SuperSpeed USB device number 3 using xhci_hcd + [ 79.475003] usb 2-2.1: LPM exit latency is zeroed, disabling LPM. + [ 79.475389] usb 2-2.1: New USB device found, idVendor=1d6b, idProduct=0004 + [ 79.475390] usb 2-2.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 + [ 79.475391] usb 2-2.1: Product: Remote GDB + [ 79.475392] usb 2-2.1: Manufacturer: Linux + [ 79.475393] usb 2-2.1: SerialNumber: 0001 + [ 79.660368] usb_debug 2-2.1:1.0: xhci_dbc converter detected + [ 79.660439] usb 2-2.1: xhci_dbc converter now attached to ttyUSB0 + +You can simply verify whether it works by:: + + # On target side + root@target: echo "Hello world" > /dev/ttyGS0 + + # On host side + root@host: cat /dev/ttyUSB0 + Hello world + + # vice versa + +You have a workable serial link over USB now. -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/4] usb: xhci: Add debug capability support in xhci
Hi, This series is for xHCI debug capability (spec section 7.6.8) support in the xHCI driver. xHCI compatible USB host controllers(i.e. super-speed USB3 controllers) can be implemented with the Debug Capability(DbC). It presents a debug device which is fully compliant with the USB framework and provides the equivalent of a very high performance full-duplex serial link. The debug capability operation model and registers interface are defined in 7.6.8 of the xHCI specification, revision 1.1. The DbC debug device shares a root port with the xHCI host. By default, the debug capability is disabled and the root port is assigned to xHCI. When the DbC is enabled, the root port will be assigned to the DbC debug device, and the xHCI sees nothing on this port. This implementation uses a sysfs node named under the xHCI device to manage the enabling and disabling of the debug capability. When the debug capability is enabled, it will present a debug device through the debug port. This debug device is fully compliant with the USB3 framework, and it can be enumerated by a debug host on the other end of the USB link. As soon as the debug device is configured, a TTY serial device named /dev/ttyGSn will be created. One use of this link is running a login service on the debug target. Hence it can be remote accessed by a debug host. Another use case can probably be found in servers. It provides a peer-to-peer USB link between two host-only machines. This provides a reasonable out-of-band communication method between two servers. Best regards, Lu Baolu --- Change log: v1->v2: - Add a new patch to move u_serial.c from drivers/usb/gadget/function to drivers/usb/common/ and move u_serial.h to include/linux/usb/. Lu Baolu (4): usb: xhci: Make some static functions global usb: common: Move u_serial from gadget/function to usb/common usb: xhci: Add DbC support in xHCI driver usb: doc: Update document for USB3 debug port usage .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd | 25 + Documentation/driver-api/usb/usb3-debug-port.rst | 68 + drivers/usb/Kconfig|3 + drivers/usb/Makefile |3 +- drivers/usb/common/Makefile|1 + drivers/usb/common/u_serial.c | 1604 +++ drivers/usb/gadget/Kconfig |3 - drivers/usb/gadget/function/Makefile |1 - drivers/usb/gadget/function/f_acm.c|4 +- drivers/usb/gadget/function/f_obex.c |4 +- drivers/usb/gadget/function/f_serial.c |3 +- drivers/usb/gadget/function/u_serial.c | 1606 drivers/usb/gadget/function/u_serial.h | 71 - drivers/usb/gadget/legacy/acm_ms.c |3 +- drivers/usb/gadget/legacy/cdc2.c |2 +- drivers/usb/gadget/legacy/dbgp.c |3 +- drivers/usb/gadget/legacy/multi.c |2 +- drivers/usb/gadget/legacy/nokia.c |2 +- drivers/usb/gadget/legacy/serial.c |3 +- drivers/usb/host/Kconfig |9 + drivers/usb/host/Makefile |5 + drivers/usb/host/xhci-dbgcap.c | 1100 ++ drivers/usb/host/xhci-dbgcap.h | 194 +++ drivers/usb/host/xhci-mem.c| 94 +- drivers/usb/host/xhci-ring.c |4 +- drivers/usb/host/xhci-trace.h | 65 + drivers/usb/host/xhci.c| 10 + drivers/usb/host/xhci.h| 17 +- include/linux/usb/gadget.h | 52 + include/linux/usb/u_serial.h | 71 + 30 files changed, 3290 insertions(+), 1742 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd create mode 100644 drivers/usb/common/u_serial.c delete mode 100644 drivers/usb/gadget/function/u_serial.c delete mode 100644 drivers/usb/gadget/function/u_serial.h create mode 100644 drivers/usb/host/xhci-dbgcap.c create mode 100644 drivers/usb/host/xhci-dbgcap.h create mode 100644 include/linux/usb/u_serial.h -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] usb: xhci: Make some static functions global
This patch makes some static functions global to avoid duplications
in different files. These functions can be used in the implementation
of xHCI debug capability. There is no functional change.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-mem.c | 94 ++--
drivers/usb/host/xhci-ring.c | 4 +-
drivers/usb/host/xhci.h | 16 +++-
3 files changed, 72 insertions(+), 42 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 2a82c92..1ccb1c5 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -368,7 +368,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd
*xhci,
* Set the end flag and the cycle toggle bit on the last segment.
* See section 4.9.1 and figures 15 and 16.
*/
-static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
+struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
unsigned int num_segs, unsigned int cycle_state,
enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
{
@@ -467,7 +467,7 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct
xhci_ring *ring,
#define CTX_SIZE(_hcc) (HCC_64BYTE_CONTEXT(_hcc) ? 64 : 32)
-static struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd
*xhci,
+struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd *xhci,
int type, gfp_t flags)
{
struct xhci_container_ctx *ctx;
@@ -492,7 +492,7 @@ static struct xhci_container_ctx
*xhci_alloc_container_ctx(struct xhci_hcd *xhci
return ctx;
}
-static void xhci_free_container_ctx(struct xhci_hcd *xhci,
+void xhci_free_container_ctx(struct xhci_hcd *xhci,
struct xhci_container_ctx *ctx)
{
if (!ctx)
@@ -1762,21 +1762,61 @@ void xhci_free_command(struct xhci_hcd *xhci,
kfree(command);
}
+int xhci_alloc_erst(struct xhci_hcd *xhci,
+ struct xhci_ring *evt_ring,
+ struct xhci_erst *erst,
+ gfp_t flags)
+{
+ size_t size;
+ unsigned int val;
+ struct xhci_segment *seg;
+ struct xhci_erst_entry *entry;
+
+ size = sizeof(struct xhci_erst_entry) * evt_ring->num_segs;
+ erst->entries = dma_alloc_coherent(xhci_to_hcd(xhci)->self.sysdev,
+ size,
+ &erst->erst_dma_addr,
+ flags);
+ if (!erst->entries)
+ return -ENOMEM;
+
+ memset(erst->entries, 0, size);
+ erst->num_entries = evt_ring->num_segs;
+
+ seg = evt_ring->first_seg;
+ for (val = 0; val < evt_ring->num_segs; val++) {
+ entry = &erst->entries[val];
+ entry->seg_addr = cpu_to_le64(seg->dma);
+ entry->seg_size = cpu_to_le32(TRBS_PER_SEGMENT);
+ entry->rsvd = 0;
+ seg = seg->next;
+ }
+
+ return 0;
+}
+
+void xhci_free_erst(struct xhci_hcd *xhci, struct xhci_erst *erst)
+{
+ size_t size;
+ struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
+
+ size = sizeof(struct xhci_erst_entry) * (erst->num_entries);
+ if (erst->entries)
+ dma_free_coherent(dev, size,
+ erst->entries,
+ erst->erst_dma_addr);
+ erst->entries = NULL;
+}
+
void xhci_mem_cleanup(struct xhci_hcd *xhci)
{
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
- int size;
int i, j, num_ports;
cancel_delayed_work_sync(&xhci->cmd_timer);
- /* Free the Event Ring Segment Table and the actual Event Ring */
- size = sizeof(struct xhci_erst_entry)*(xhci->erst.num_entries);
- if (xhci->erst.entries)
- dma_free_coherent(dev, size,
- xhci->erst.entries, xhci->erst.erst_dma_addr);
- xhci->erst.entries = NULL;
- xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed ERST");
+ xhci_free_erst(xhci, &xhci->erst);
+
if (xhci->event_ring)
xhci_ring_free(xhci, xhci->event_ring);
xhci->event_ring = NULL;
@@ -2313,9 +2353,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
unsigned intval, val2;
u64 val_64;
- struct xhci_segment *seg;
- u32 page_size, temp;
- int i;
+ u32 page_size, temp;
+ int i, ret;
INIT_LIST_HEAD(&xhci->cmd_list);
@@ -2454,32 +2493,9 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
if (xhci_check_trb_in_td_math(xhci) < 0)
goto fail;
- xhci->erst.entries =
[PATCH v2 3/4] usb: xhci: Add DbC support in xHCI driver
xHCI compatible USB host controllers(i.e. super-speed USB3 controllers) can be implemented with the Debug Capability(DbC). It presents a debug device which is fully compliant with the USB framework and provides the equivalent of a very high performance full-duplex serial link. The debug capability operation model and registers interface are defined in 7.6.8 of the xHCI specification, revision 1.1. The DbC debug device shares a root port with the xHCI host. By default, the debug capability is disabled and the root port is assigned to xHCI. When the DbC is enabled, the root port will be assigned to the DbC debug device, and the xHCI sees nothing on this port. This implementation uses a sysfs node named under the xHCI device to manage the enabling and disabling of the debug capability. When the debug capability is enabled, it will present a debug device through the debug port. This debug device is fully compliant with the USB3 framework, and it can be enumerated by a debug host on the other end of the USB link. As soon as the debug device is configured, a TTY serial device named /dev/ttyGSn will be created. One use of this link is running a login service on the debug target. Hence it can be remote accessed by a debug host. Another use case can probably be found in servers. It provides a peer-to-peer USB link between two host-only machines. This provides a reasonable out-of-band communication method between two servers. Signed-off-by: Lu Baolu --- .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd | 25 + drivers/usb/host/Kconfig |9 + drivers/usb/host/Makefile |5 + drivers/usb/host/xhci-dbgcap.c | 1100 drivers/usb/host/xhci-dbgcap.h | 194 drivers/usb/host/xhci-trace.h | 65 ++ drivers/usb/host/xhci.c| 10 + drivers/usb/host/xhci.h|1 + include/linux/usb/gadget.h | 52 + 9 files changed, 1461 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd create mode 100644 drivers/usb/host/xhci-dbgcap.c create mode 100644 drivers/usb/host/xhci-dbgcap.h diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd new file mode 100644 index 000..0088aba --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd @@ -0,0 +1,25 @@ +What: /sys/bus/pci/drivers/xhci_hcd/.../dbc +Date: June 2017 +Contact: Lu Baolu +Description: + xHCI compatible USB host controllers (i.e. super-speed + USB3 controllers) are often implemented with the Debug + Capability (DbC). It can present a debug device which + is fully compliant with the USB framework and provides + the equivalent of a very high performance full-duplex + serial link for debug purpose. + + The DbC debug device shares a root port with xHCI host. + When the DbC is enabled, the root port will be assigned + to the Debug Capability. Otherwise, it will be assigned + to xHCI. + + Writing "enable" to this attribute will enable the DbC + functionality and the shared root port will be assigned + to the DbC device. Writing "disable" to this attribute + will disable the DbC functionality and the shared root + port will roll back to the xHCI. + + Reading this attribute gives the state of the DbC. It + can be one of the following states: disabled, enabled, + initialized, connected, configured and stalled. diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index fa5692d..968a196 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -27,6 +27,15 @@ config USB_XHCI_HCD module will be called xhci-hcd. if USB_XHCI_HCD +config USB_XHCI_DBGCAP + bool "xHCI support for debug capability" + depends on TTY + select USB_U_SERIAL + ---help--- + Say 'Y' to enable the support for the xHCI debug capability. Make + sure that your xHCI host supports the extended debug capability and + you want a TTY serial device based on the xHCI debug capability + before enabling this option. If unsure, say 'N'. config USB_XHCI_PCI tristate diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile index cf2691f..4629d20 100644 --- a/drivers/usb/host/Makefile +++ b/drivers/usb/host/Makefile @@ -13,6 +13,11 @@ fhci-$(CONFIG_FHCI_DEBUG) += fhci-dbg.o xhci-hcd-y := xhci.o xhci-mem.o xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o xhci-hcd-y += xhci-trace.o + +ifneq ($(CONFIG_USB_XHCI_DBGCAP), ) +
Re: [PATCH v2 2/4] usb: common: Move u_serial from gadget/function to usb/common
Hi, On 08/07/2017 04:13 PM, Felipe Balbi wrote: > Hi, > > Lu Baolu writes: >> The component u_serial provides a glue layer between TTY layer >> and a USB gadget device needed to provide a basic serial port >> functionality. Currently, u_serial sits under gadget/function >> and depends on CONFIG_USB_GADGET to be compiled and used. >> >> Most of the serial gadget devices are based on a UDC (USB device >> controller) and implemented by making use of the Linux gadget >> frameworks. But we are facing other implementions as well. One >> example can be found with xHCI debug capability. The xHCI debug >> capability implements a serial gadget with hardware and firmware, >> and provides an interface similar with xHCI host for submitting >> and reaping the transfer requests. >> >> In order to make better use of u_serial when implementing xHCI >> debug capability in xHCI driver, this patch moves u_serial.c >> from gadget/function to usb/common, and moves u_serial.h from >> gadget/function to include/linux/usb. >> >> Signed-off-by: Lu Baolu > NAK, u_serial uses the gadget API. It's definitely not COMMON. > Okay. It seems that I can't use u_serial anyway. I will implement a new tty glue for my case. Best regards, Lu Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] usb: common: Move u_serial from gadget/function to usb/common
Hi, On 08/08/2017 02:14 PM, Felipe Balbi wrote: > Hi, > > Lu Baolu writes: >>> Lu Baolu writes: >>>> The component u_serial provides a glue layer between TTY layer >>>> and a USB gadget device needed to provide a basic serial port >>>> functionality. Currently, u_serial sits under gadget/function >>>> and depends on CONFIG_USB_GADGET to be compiled and used. >>>> >>>> Most of the serial gadget devices are based on a UDC (USB device >>>> controller) and implemented by making use of the Linux gadget >>>> frameworks. But we are facing other implementions as well. One >>>> example can be found with xHCI debug capability. The xHCI debug >>>> capability implements a serial gadget with hardware and firmware, >>>> and provides an interface similar with xHCI host for submitting >>>> and reaping the transfer requests. >>>> >>>> In order to make better use of u_serial when implementing xHCI >>>> debug capability in xHCI driver, this patch moves u_serial.c >>>> from gadget/function to usb/common, and moves u_serial.h from >>>> gadget/function to include/linux/usb. >>>> >>>> Signed-off-by: Lu Baolu >>> NAK, u_serial uses the gadget API. It's definitely not COMMON. >>> >> Okay. It seems that I can't use u_serial anyway. I will implement >> a new tty glue for my case. > have you looked at drivers/usb/serial/? > Yes, I've checked that. I think usb-serial framework is too complex for this case. XHCI debug capability is a simple gadget attached to xHCI host. Best regards, Lu Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] usb: xhci: Disable slot even virt-dev is null
Hi,
On 08/09/2017 03:58 PM, Mathias Nyman wrote:
> On 27.07.2017 05:21, Lu Baolu wrote:
>> xhci_disable_slot() is a helper for disabling a slot when a device
>> goes away or recovers from error situations. Currently, it checks
>> the corespoding virt-dev pointer and returns directly (w/o issuing
>> disable slot command) if it's null.
>>
>> This is unnecessary and will cause problems in case where virt-dev
>> allocation fails and xhci_disable_slot() is called to roll back the
>> hardware state. Refer to the implementation of xhci_alloc_dev().
>>
>
> True, checking for xhci->devs[slot_id] doesn't work if xhci_alloc_dev()
> failed xhci_alloc_virt_device() and calls xhci_disable_slot,
>
> but the virt dev check is needed for test mode, which will just try
> to disable all slots from 1 to HCS_MAX_SLOTS:
>
> xhci_enter_test_mode(..)
>/* Disable all Device Slots */
> for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
> retval = xhci_disable_slot(xhci, NULL, i);
>
>
So, how about checking virt dev before this invoking?
@@ -612,10 +612,12 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
xhci_dbg(xhci, "Disable all slots\n");
spin_unlock_irqrestore(&xhci->lock, *flags);
for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
- retval = xhci_disable_slot(xhci, NULL, i);
- if (retval)
- xhci_err(xhci, "Failed to disable slot %d, %d. Enter
test mode anyway\n",
-i, retval);
+ if (xhci->devs[i]) {
+ retval = xhci_disable_slot(xhci, NULL, i);
+ if (retval)
+ xhci_err(xhci, "Failed to disable slot %d, %d.
Enter test mode anyway\n",
+i, retval);
+ }
}
spin_lock_irqsave(&xhci->lock, *flags);
/* Put all ports to the Disable state by clear PP */
Best regards,
Lu Baolu
>
>> This patch removes lines to check virt-dev in xhci_disable_slot().
>>
>> Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
>> Cc: Guoqing Zhang
>> Signed-off-by: Lu Baolu
>> ---
>> drivers/usb/host/xhci.c | 4
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index b2ff1ff..e69073f 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -3567,11 +3567,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct
>> xhci_command *command,
>> unsigned long flags;
>> u32 state;
>> int ret = 0;
>> -struct xhci_virt_device *virt_dev;
>>
>> -virt_dev = xhci->devs[slot_id];
>> -if (!virt_dev)
>> -return -EINVAL;
>> if (!command)
>> command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
>> if (!command)
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] usb: xhci: Disable slot even virt-dev is null
Hi,
On 08/10/2017 06:00 PM, Mathias Nyman wrote:
> On 10.08.2017 03:35, Lu Baolu wrote:
>> Hi,
>>
>> On 08/09/2017 03:58 PM, Mathias Nyman wrote:
>>> On 27.07.2017 05:21, Lu Baolu wrote:
>>>> xhci_disable_slot() is a helper for disabling a slot when a device
>>>> goes away or recovers from error situations. Currently, it checks
>>>> the corespoding virt-dev pointer and returns directly (w/o issuing
>>>> disable slot command) if it's null.
>>>>
>>>> This is unnecessary and will cause problems in case where virt-dev
>>>> allocation fails and xhci_disable_slot() is called to roll back the
>>>> hardware state. Refer to the implementation of xhci_alloc_dev().
>>>>
>>>
>>> True, checking for xhci->devs[slot_id] doesn't work if xhci_alloc_dev()
>>> failed xhci_alloc_virt_device() and calls xhci_disable_slot,
>>>
>>> but the virt dev check is needed for test mode, which will just try
>>> to disable all slots from 1 to HCS_MAX_SLOTS:
>>>
>>> xhci_enter_test_mode(..)
>>> /* Disable all Device Slots */
>>> for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
>>> retval = xhci_disable_slot(xhci, NULL, i);
>>>
>>>
>>
>> So, how about checking virt dev before this invoking?
>>
>> @@ -612,10 +612,12 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
>> xhci_dbg(xhci, "Disable all slots\n");
>> spin_unlock_irqrestore(&xhci->lock, *flags);
>> for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
>> - retval = xhci_disable_slot(xhci, NULL, i);
>> - if (retval)
>> - xhci_err(xhci, "Failed to disable slot %d, %d. Enter
>> test mode anyway\n",
>> -i, retval);
>> + if (xhci->devs[i]) {
>> + retval = xhci_disable_slot(xhci, NULL, i);
>> + if (retval)
>> + xhci_err(xhci, "Failed to disable slot %d,
>> %d. Enter test mode anyway\n",
>> +i, retval);
>> + }
>
>
> Yes, something like this will work
>
Okay, I will submit a v3 to include this change.
Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/5] usb: xhci: Disable slot even virt-dev is null
xhci_disable_slot() is a helper for disabling a slot when a device
goes away or recovers from error situations. Currently, it checks
the corespoding virt-dev pointer and returns directly (w/o issuing
disable slot command) if it's null.
This is unnecessary and will cause problems in case where virt-dev
allocation fails and xhci_disable_slot() is called to roll back the
hardware state. Refer to the implementation of xhci_alloc_dev().
This patch removes lines to check virt-dev in xhci_disable_slot().
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-hub.c | 3 +++
drivers/usb/host/xhci.c | 4
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 00721e8..6fcb98d 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -612,6 +612,9 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
xhci_dbg(xhci, "Disable all slots\n");
spin_unlock_irqrestore(&xhci->lock, *flags);
for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
+ if (!xhci->devs[i])
+ continue;
+
retval = xhci_disable_slot(xhci, NULL, i);
if (retval)
xhci_err(xhci, "Failed to disable slot %d, %d. Enter
test mode anyway\n",
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b2ff1ff..e69073f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3567,11 +3567,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct
xhci_command *command,
unsigned long flags;
u32 state;
int ret = 0;
- struct xhci_virt_device *virt_dev;
- virt_dev = xhci->devs[slot_id];
- if (!virt_dev)
- return -EINVAL;
if (!command)
command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
if (!command)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/5] usb: xhci: Handle USB transaction error on address command
Xhci driver handles USB transaction errors on transfer events, but transaction errors are possible on address device command completion events as well. The xHCI specification (section 4.6.5) says: A USB Transaction Error Completion Code for an Address Device Command may be due to a Stall response from a device. Software should issue a Disable Slot Command for the Device Slot then an Enable Slot Command to recover from this error. This patch handles USB transaction errors on address command completion events. The related discussion threads can be found through below links. http://marc.info/?l=linux-usb&m=149362010728921&w=2 http://marc.info/?l=linux-usb&m=149252752825755&w=2 Suggested-by: Mathias Nyman Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index d6b728d..95780f8 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3822,6 +3822,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, break; case COMP_USB_TRANSACTION_ERROR: dev_warn(&udev->dev, "Device not responding to setup %s.\n", act); + + ret = xhci_disable_slot(xhci, udev->slot_id); + if (!ret) + xhci_alloc_dev(hcd, udev); + ret = -EPROTO; break; case COMP_INCOMPATIBLE_DEVICE_ERROR: -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/5] usb: xhci: Return error when host is dead in xhci_disable_slot()
xhci_disable_slot() is a helper for disabling a slot when a device
goes away or recovers from error situations. Currently, it returns
success when it sees a dead host. This is not the right way to go.
It should return error and let the invoker know that disable slot
command was failed due to a dead host.
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 2df601e..d6b728d 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3568,10 +3568,9 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
state = readl(&xhci->op_regs->status);
if (state == 0x || (xhci->xhc_state & XHCI_STATE_DYING) ||
(xhci->xhc_state & XHCI_STATE_HALTED)) {
- xhci_free_virt_device(xhci, slot_id);
spin_unlock_irqrestore(&xhci->lock, flags);
kfree(command);
- return ret;
+ return -ENODEV;
}
ret = xhci_queue_slot_control(xhci, command, TRB_DISABLE_SLOT,
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/5] usb: xhci: Handle USB transaction error on address command
Xhci driver handles USB transaction errors on transfer events, but transaction errors are possible on address device command completion events as well. The xHCI specification (section 4.6.5) says: A USB Transaction Error Completion Code for an Address Device Command may be due to a Stall response from a device. Software should issue a Disable Slot Command for the Device Slot then an Enable Slot Command to recover from this error. The related discussion threads can be found through below links. http://marc.info/?l=linux-usb&m=149362010728921&w=2 http://marc.info/?l=linux-usb&m=149252752825755&w=2 This patch set includes some fixes in xhci_disable_slot() as well which will be used to handle USB transaction error on address command. --- Change log: v1->v2: - Add 4 fixes in xhci_disable_slot which will be used to handle USB transaction error on address command. v2->v3: - Add checking virt dev for test mode in PATCH 1/5. Lu Baolu (5): usb: xhci: Disable slot even virt-dev is null usb: xhci: Fix potential memory leak in xhci_disable_slot() usb: xhci: Fix memory leak when xhci_disable_slot() returns error usb: xhci: Return error when host is dead in xhci_disable_slot() usb: xhci: Handle USB transaction error on address command drivers/usb/host/xhci-hub.c | 5 - drivers/usb/host/xhci.c | 52 ++--- drivers/usb/host/xhci.h | 3 +-- 3 files changed, 26 insertions(+), 34 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/5] usb: xhci: Fix memory leak when xhci_disable_slot() returns error
If xhci_disable_slot() returns success, a disable slot command trb was queued in the command ring. The command completion handler will free the virtual device data structure associated with the slot. On the other hand, when xhci_disable_slot() returns error, the invokers should take the responsibilities to free the slot related data structure. Otherwise, memory leakage happens. Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index cb2461a..2df601e 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3547,11 +3547,9 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) del_timer_sync(&virt_dev->eps[i].stop_cmd_timer); } - xhci_disable_slot(xhci, udev->slot_id); - /* -* Event command completion handler will free any data structures -* associated with the slot. XXX Can free sleep? -*/ + ret = xhci_disable_slot(xhci, udev->slot_id); + if (ret) + xhci_free_virt_device(xhci, udev->slot_id); } int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id) @@ -3697,7 +3695,11 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) return 1; disable_slot: - return xhci_disable_slot(xhci, udev->slot_id); + ret = xhci_disable_slot(xhci, udev->slot_id); + if (ret) + xhci_free_virt_device(xhci, udev->slot_id); + + return 0; } /* -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/5] usb: xhci: Fix potential memory leak in xhci_disable_slot()
xhci_disable_slot() allows the invoker to pass a command pointer
as paramenter. Otherwise, it will allocate one. This will cause
memory leak when a command structure was allocated inside of this
function while queuing command trb fails. Another problem comes up
when the invoker passed a command pointer, but xhci_disable_slot()
frees it when it detects a dead host.
This patch fixes these two problems by removing the command parameter
from xhci_disable_slot().
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-hub.c | 2 +-
drivers/usb/host/xhci.c | 30 +-
drivers/usb/host/xhci.h | 3 +--
3 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 6fcb98d..daaf155 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -615,7 +615,7 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
if (!xhci->devs[i])
continue;
- retval = xhci_disable_slot(xhci, NULL, i);
+ retval = xhci_disable_slot(xhci, i);
if (retval)
xhci_err(xhci, "Failed to disable slot %d, %d. Enter
test mode anyway\n",
i, retval);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e69073f..cb2461a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3519,11 +3519,6 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
struct xhci_virt_device *virt_dev;
struct xhci_slot_ctx *slot_ctx;
int i, ret;
- struct xhci_command *command;
-
- command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
- if (!command)
- return;
#ifndef CONFIG_USB_DEFAULT_PERSIST
/*
@@ -3539,10 +3534,8 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
/* If the host is halted due to driver unload, we still need to free the
* device.
*/
- if (ret <= 0 && ret != -ENODEV) {
- kfree(command);
+ if (ret <= 0 && ret != -ENODEV)
return;
- }
virt_dev = xhci->devs[udev->slot_id];
slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx);
@@ -3554,22 +3547,21 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
del_timer_sync(&virt_dev->eps[i].stop_cmd_timer);
}
- xhci_disable_slot(xhci, command, udev->slot_id);
+ xhci_disable_slot(xhci, udev->slot_id);
/*
* Event command completion handler will free any data structures
* associated with the slot. XXX Can free sleep?
*/
}
-int xhci_disable_slot(struct xhci_hcd *xhci, struct xhci_command *command,
- u32 slot_id)
+int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
{
+ struct xhci_command *command;
unsigned long flags;
u32 state;
int ret = 0;
- if (!command)
- command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
+ command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
if (!command)
return -ENOMEM;
@@ -3588,7 +3580,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct
xhci_command *command,
slot_id);
if (ret) {
spin_unlock_irqrestore(&xhci->lock, flags);
- xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
+ kfree(command);
return ret;
}
xhci_ring_cmd_db(xhci);
@@ -3663,6 +3655,8 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device
*udev)
return 0;
}
+ xhci_free_command(xhci, command);
+
if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK)) {
spin_lock_irqsave(&xhci->lock, flags);
ret = xhci_reserve_host_control_ep_resources(xhci);
@@ -3698,18 +3692,12 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct
usb_device *udev)
pm_runtime_get_noresume(hcd->self.controller);
#endif
-
- xhci_free_command(xhci, command);
/* Is this a LS or FS device under a HS hub? */
/* Hub or peripherial? */
return 1;
disable_slot:
- /* Disable slot, if we can do it without mem alloc */
- kfree(command->completion);
- command->completion = NULL;
- command->status = 0;
- return xhci_disable_slot(xhci, command, udev->slot_id);
+ return xhci_disable_slot(xhci, udev->slot_id);
}
/*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index e3e9352..6325d58 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
Re: [PATCH v3 5/5] usb: xhci: Handle USB transaction error on address command
Hi,
On 08/15/2017 07:30 PM, Mathias Nyman wrote:
> On 11.08.2017 05:41, Lu Baolu wrote:
>> Xhci driver handles USB transaction errors on transfer events,
>> but transaction errors are possible on address device command
>> completion events as well.
>>
>> The xHCI specification (section 4.6.5) says: A USB Transaction
>> Error Completion Code for an Address Device Command may be due
>> to a Stall response from a device. Software should issue a Disable
>> Slot Command for the Device Slot then an Enable Slot Command to
>> recover from this error.
>>
>> This patch handles USB transaction errors on address command
>> completion events. The related discussion threads can be found
>> through below links.
>>
>> http://marc.info/?l=linux-usb&m=149362010728921&w=2
>> http://marc.info/?l=linux-usb&m=149252752825755&w=2
>>
>> Suggested-by: Mathias Nyman
>> Signed-off-by: Lu Baolu
>> ---
>> drivers/usb/host/xhci.c | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index d6b728d..95780f8 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -3822,6 +3822,11 @@ static int xhci_setup_device(struct usb_hcd *hcd,
>> struct usb_device *udev,
>> break;
>> case COMP_USB_TRANSACTION_ERROR:
>> dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
>> +
>> +ret = xhci_disable_slot(xhci, udev->slot_id);
>> +if (!ret)
>> +xhci_alloc_dev(hcd, udev);
>
> Might be a xhci->mutex locking issue here,
> both xhci_setup_device() and xhci_alloc_dev() take xhci->mutex
>
I will apply xhci->mutex in this patch for code consistency, but I think
we can drop xhci->mutex (in a separated patch) anyway.
xhci->mutex was introduced to protect two race sources of xhci->slot_id
and xhci->addr_dev by below commit:
commit a00918d0521df1c7a2ec9143142a3ea998c8526d
Author: Chris Bainbridge
Date: Tue May 19 16:30:51 2015 +0300
usb: host: xhci: add mutex for non-thread-safe data
Regression in commit 638139eb95d2 ("usb: hub: allow to process more usb
hub events in parallel")
The regression resulted in intermittent failure to initialise a 10-port
hub (with three internal VL812 4-port hub controllers) on boot, with a
failure rate of around 8%, due to multiple race conditions when
accessing addr_dev and slot_id in struct xhci_hcd.
This regression also exposed a problem with xhci_setup_device, which
"should be protected by the usb_address0_mutex" but no longer is due to
commit 6fecd4f2a58c ("USB: separate usb_address0 mutexes for each bus")
With separate buses (and locks) it is no longer the case that a single
lock will protect xhci_setup_device from accesses by two parallel
threads processing events on the two buses.
Fix this by adding a mutex to protect addr_dev and slot_id in struct
xhci_hcd, and by making the assignment of slot_id atomic.
[--cut---]
We have already removed these two race sources after that by below
two commits:
c2d3d49 usb: xhci: move slot_id from xhci_hcd to xhci_command structure
87e44f2 usb: xhci: remove the use of xhci->addr_dev
So we don't need xhci->mutex any more. I will try to do this in a separated
patch with more tests. For now, I will add xhci->mutex for code consistency.
Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 4/5] usb: xhci: Return error when host is dead in xhci_disable_slot()
xhci_disable_slot() is a helper for disabling a slot when a device
goes away or recovers from error situations. Currently, it returns
success when it sees a dead host. This is not the right way to go.
It should return error and let the invoker know that disable slot
command was failed due to a dead host.
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 2df601e..d6b728d 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3568,10 +3568,9 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
state = readl(&xhci->op_regs->status);
if (state == 0x || (xhci->xhc_state & XHCI_STATE_DYING) ||
(xhci->xhc_state & XHCI_STATE_HALTED)) {
- xhci_free_virt_device(xhci, slot_id);
spin_unlock_irqrestore(&xhci->lock, flags);
kfree(command);
- return ret;
+ return -ENODEV;
}
ret = xhci_queue_slot_control(xhci, command, TRB_DISABLE_SLOT,
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/5] usb: xhci: Fix memory leak when xhci_disable_slot() returns error
If xhci_disable_slot() returns success, a disable slot command trb was queued in the command ring. The command completion handler will free the virtual device data structure associated with the slot. On the other hand, when xhci_disable_slot() returns error, the invokers should take the responsibilities to free the slot related data structure. Otherwise, memory leakage happens. Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index cb2461a..2df601e 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3547,11 +3547,9 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) del_timer_sync(&virt_dev->eps[i].stop_cmd_timer); } - xhci_disable_slot(xhci, udev->slot_id); - /* -* Event command completion handler will free any data structures -* associated with the slot. XXX Can free sleep? -*/ + ret = xhci_disable_slot(xhci, udev->slot_id); + if (ret) + xhci_free_virt_device(xhci, udev->slot_id); } int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id) @@ -3697,7 +3695,11 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) return 1; disable_slot: - return xhci_disable_slot(xhci, udev->slot_id); + ret = xhci_disable_slot(xhci, udev->slot_id); + if (ret) + xhci_free_virt_device(xhci, udev->slot_id); + + return 0; } /* -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 5/5] usb: xhci: Handle USB transaction error on address command
Xhci driver handles USB transaction errors on transfer events, but transaction errors are possible on address device command completion events as well. The xHCI specification (section 4.6.5) says: A USB Transaction Error Completion Code for an Address Device Command may be due to a Stall response from a device. Software should issue a Disable Slot Command for the Device Slot then an Enable Slot Command to recover from this error. This patch handles USB transaction errors on address command completion events. The related discussion threads can be found through below links. http://marc.info/?l=linux-usb&m=149362010728921&w=2 http://marc.info/?l=linux-usb&m=149252752825755&w=2 Suggested-by: Mathias Nyman Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index d6b728d..c8a64d2 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3822,6 +3822,13 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, break; case COMP_USB_TRANSACTION_ERROR: dev_warn(&udev->dev, "Device not responding to setup %s.\n", act); + + mutex_unlock(&xhci->mutex); + ret = xhci_disable_slot(xhci, udev->slot_id); + if (!ret) + xhci_alloc_dev(hcd, udev); + mutex_lock(&xhci->mutex); + ret = -EPROTO; break; case COMP_INCOMPATIBLE_DEVICE_ERROR: -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/5] usb: xhci: Disable slot even virt-dev is null
xhci_disable_slot() is a helper for disabling a slot when a device
goes away or recovers from error situations. Currently, it checks
the corespoding virt-dev pointer and returns directly (w/o issuing
disable slot command) if it's null.
This is unnecessary and will cause problems in case where virt-dev
allocation fails and xhci_disable_slot() is called to roll back the
hardware state. Refer to the implementation of xhci_alloc_dev().
This patch removes lines to check virt-dev in xhci_disable_slot().
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-hub.c | 3 +++
drivers/usb/host/xhci.c | 4
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 00721e8..6fcb98d 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -612,6 +612,9 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
xhci_dbg(xhci, "Disable all slots\n");
spin_unlock_irqrestore(&xhci->lock, *flags);
for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
+ if (!xhci->devs[i])
+ continue;
+
retval = xhci_disable_slot(xhci, NULL, i);
if (retval)
xhci_err(xhci, "Failed to disable slot %d, %d. Enter
test mode anyway\n",
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b2ff1ff..e69073f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3567,11 +3567,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct
xhci_command *command,
unsigned long flags;
u32 state;
int ret = 0;
- struct xhci_virt_device *virt_dev;
- virt_dev = xhci->devs[slot_id];
- if (!virt_dev)
- return -EINVAL;
if (!command)
command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
if (!command)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/5] usb: xhci: Fix potential memory leak in xhci_disable_slot()
xhci_disable_slot() allows the invoker to pass a command pointer
as paramenter. Otherwise, it will allocate one. This will cause
memory leak when a command structure was allocated inside of this
function while queuing command trb fails. Another problem comes up
when the invoker passed a command pointer, but xhci_disable_slot()
frees it when it detects a dead host.
This patch fixes these two problems by removing the command parameter
from xhci_disable_slot().
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-hub.c | 2 +-
drivers/usb/host/xhci.c | 30 +-
drivers/usb/host/xhci.h | 3 +--
3 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 6fcb98d..daaf155 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -615,7 +615,7 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
if (!xhci->devs[i])
continue;
- retval = xhci_disable_slot(xhci, NULL, i);
+ retval = xhci_disable_slot(xhci, i);
if (retval)
xhci_err(xhci, "Failed to disable slot %d, %d. Enter
test mode anyway\n",
i, retval);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e69073f..cb2461a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3519,11 +3519,6 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
struct xhci_virt_device *virt_dev;
struct xhci_slot_ctx *slot_ctx;
int i, ret;
- struct xhci_command *command;
-
- command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
- if (!command)
- return;
#ifndef CONFIG_USB_DEFAULT_PERSIST
/*
@@ -3539,10 +3534,8 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
/* If the host is halted due to driver unload, we still need to free the
* device.
*/
- if (ret <= 0 && ret != -ENODEV) {
- kfree(command);
+ if (ret <= 0 && ret != -ENODEV)
return;
- }
virt_dev = xhci->devs[udev->slot_id];
slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx);
@@ -3554,22 +3547,21 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
del_timer_sync(&virt_dev->eps[i].stop_cmd_timer);
}
- xhci_disable_slot(xhci, command, udev->slot_id);
+ xhci_disable_slot(xhci, udev->slot_id);
/*
* Event command completion handler will free any data structures
* associated with the slot. XXX Can free sleep?
*/
}
-int xhci_disable_slot(struct xhci_hcd *xhci, struct xhci_command *command,
- u32 slot_id)
+int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
{
+ struct xhci_command *command;
unsigned long flags;
u32 state;
int ret = 0;
- if (!command)
- command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
+ command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
if (!command)
return -ENOMEM;
@@ -3588,7 +3580,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct
xhci_command *command,
slot_id);
if (ret) {
spin_unlock_irqrestore(&xhci->lock, flags);
- xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
+ kfree(command);
return ret;
}
xhci_ring_cmd_db(xhci);
@@ -3663,6 +3655,8 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device
*udev)
return 0;
}
+ xhci_free_command(xhci, command);
+
if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK)) {
spin_lock_irqsave(&xhci->lock, flags);
ret = xhci_reserve_host_control_ep_resources(xhci);
@@ -3698,18 +3692,12 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct
usb_device *udev)
pm_runtime_get_noresume(hcd->self.controller);
#endif
-
- xhci_free_command(xhci, command);
/* Is this a LS or FS device under a HS hub? */
/* Hub or peripherial? */
return 1;
disable_slot:
- /* Disable slot, if we can do it without mem alloc */
- kfree(command->completion);
- command->completion = NULL;
- command->status = 0;
- return xhci_disable_slot(xhci, command, udev->slot_id);
+ return xhci_disable_slot(xhci, udev->slot_id);
}
/*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index e3e9352..6325d58 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
[PATCH v4 0/5] usb: xhci: Handle USB transaction error on address command
Xhci driver handles USB transaction errors on transfer events, but transaction errors are possible on address device command completion events as well. The xHCI specification (section 4.6.5) says: A USB Transaction Error Completion Code for an Address Device Command may be due to a Stall response from a device. Software should issue a Disable Slot Command for the Device Slot then an Enable Slot Command to recover from this error. The related discussion threads can be found through below links. http://marc.info/?l=linux-usb&m=149362010728921&w=2 http://marc.info/?l=linux-usb&m=149252752825755&w=2 This patch set includes some fixes in xhci_disable_slot() as well which will be used to handle USB transaction error on address command. --- Change log: v1->v2: - Add 4 fixes in xhci_disable_slot which will be used to handle USB transaction error on address command. v2->v3: - Add checking virt dev for test mode in PATCH 1/5. v3->v4: - Resolve xhci->mutex locking issue in 5/5. Lu Baolu (5): usb: xhci: Disable slot even virt-dev is null usb: xhci: Fix potential memory leak in xhci_disable_slot() usb: xhci: Fix memory leak when xhci_disable_slot() returns error usb: xhci: Return error when host is dead in xhci_disable_slot() usb: xhci: Handle USB transaction error on address command drivers/usb/host/xhci-hub.c | 5 - drivers/usb/host/xhci.c | 54 +++-- drivers/usb/host/xhci.h | 3 +-- 3 files changed, 28 insertions(+), 34 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] usb: xhci: Handle USB transaction error on address command
Hi, On 08/18/2017 09:31 PM, Mathias Nyman wrote: > On 16.08.2017 05:15, Lu Baolu wrote: >> Hi, >> >> On 08/15/2017 07:30 PM, Mathias Nyman wrote: >>> On 11.08.2017 05:41, Lu Baolu wrote: >>>> Xhci driver handles USB transaction errors on transfer events, >>>> but transaction errors are possible on address device command >>>> completion events as well. >>>> >>>> The xHCI specification (section 4.6.5) says: A USB Transaction >>>> Error Completion Code for an Address Device Command may be due >>>> to a Stall response from a device. Software should issue a Disable >>>> Slot Command for the Device Slot then an Enable Slot Command to >>>> recover from this error. >>>> >>>> This patch handles USB transaction errors on address command >>>> completion events. The related discussion threads can be found >>>> through below links. >>>> >>>> http://marc.info/?l=linux-usb&m=149362010728921&w=2 >>>> http://marc.info/?l=linux-usb&m=149252752825755&w=2 >>>> >>>> Suggested-by: Mathias Nyman >>>> Signed-off-by: Lu Baolu >>>> --- >>>>drivers/usb/host/xhci.c | 5 + >>>>1 file changed, 5 insertions(+) >>>> >>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >>>> index d6b728d..95780f8 100644 >>>> --- a/drivers/usb/host/xhci.c >>>> +++ b/drivers/usb/host/xhci.c >>>> @@ -3822,6 +3822,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, >>>> struct usb_device *udev, >>>>break; >>>>case COMP_USB_TRANSACTION_ERROR: >>>>dev_warn(&udev->dev, "Device not responding to setup %s.\n", >>>> act); >>>> + >>>> +ret = xhci_disable_slot(xhci, udev->slot_id); >>>> +if (!ret) >>>> +xhci_alloc_dev(hcd, udev); >>> >>> Might be a xhci->mutex locking issue here, >>> both xhci_setup_device() and xhci_alloc_dev() take xhci->mutex >>> >> >> I will apply xhci->mutex in this patch for code consistency, but I think >> we can drop xhci->mutex (in a separated patch) anyway. >> >> xhci->mutex was introduced to protect two race sources of xhci->slot_id >> and xhci->addr_dev by below commit: >> >> commit a00918d0521df1c7a2ec9143142a3ea998c8526d >> usb: host: xhci: add mutex for non-thread-safe data >> >> >> c2d3d49 usb: xhci: move slot_id from xhci_hcd to xhci_command structure >> 87e44f2 usb: xhci: remove the use of xhci->addr_dev >> >> So we don't need xhci->mutex any more. I will try to do this in a separated >> patch with more tests. For now, I will add xhci->mutex for code consistency. > > Fixing xhci->mutex sounds like a good idea, and you are right, it's no longer > needed for slot_id or addr_dev. > But it's use was extended to protect xhci_stop() and xhci_setup_device() > from racing at fast xhci host hotplug/removes in this patch. > > commit 85ac90f8953a58f6a057b727bc9db97721e3fb8e > Author: Roger Quadros > Date: Mon Sep 21 17:46:12 2015 +0300 > > usb: xhci: lock mutex on xhci_stop > > Else it races with xhci_setup_device > > But I think it's safe to remove xhci->mutex from xhci_alloc_dev() > > There's no huurry with the patch 5/5 so would be nice to get that mutex > cleaned up before. Okay. Best regards, Lu Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/6] usb: xhci: Fix memory leak when xhci_disable_slot() returns error
If xhci_disable_slot() returns success, a disable slot command trb was queued in the command ring. The command completion handler will free the virtual device data structure associated with the slot. On the other hand, when xhci_disable_slot() returns error, the invokers should take the responsibilities to free the slot related data structure. Otherwise, memory leakage happens. Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 502842b..1290674 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3562,11 +3562,9 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) del_timer_sync(&virt_dev->eps[i].stop_cmd_timer); } - xhci_disable_slot(xhci, udev->slot_id); - /* -* Event command completion handler will free any data structures -* associated with the slot. XXX Can free sleep? -*/ + ret = xhci_disable_slot(xhci, udev->slot_id); + if (ret) + xhci_free_virt_device(xhci, udev->slot_id); } int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id) @@ -3714,7 +3712,11 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) return 1; disable_slot: - return xhci_disable_slot(xhci, udev->slot_id); + ret = xhci_disable_slot(xhci, udev->slot_id); + if (ret) + xhci_free_virt_device(xhci, udev->slot_id); + + return 0; } /* -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 0/6] usb: xhci: Handle USB transaction error on address command
Hi Mathias, Xhci driver handles USB transaction errors on transfer events, but transaction errors are possible on address device command completion events as well. The xHCI specification (section 4.6.5) says: A USB Transaction Error Completion Code for an Address Device Command may be due to a Stall response from a device. Software should issue a Disable Slot Command for the Device Slot then an Enable Slot Command to recover from this error. The related discussion threads can be found through below links. http://marc.info/?l=linux-usb&m=149362010728921&w=2 http://marc.info/?l=linux-usb&m=149252752825755&w=2 This patch set includes some fixes in xhci_disable_slot() as well which will be used to handle USB transaction error on address command. This patch set is based on the top of for-usb-next branch in your xhci tree. Best regards, Lu Baolu --- Change log: v1->v2: - Add 4 fixes in xhci_disable_slot which will be used to handle USB transaction error on address command. v2->v3: - Add checking virt dev for test mode in PATCH 1/5. v3->v4: - Resolve xhci->mutex locking issue in 5/5. v4->v5: - Remove xhci->mutex from xhci_alloc_dev(). Lu Baolu (6): usb: xhci: Disable slot even virt-dev is null usb: xhci: Fix potential memory leak in xhci_disable_slot() usb: xhci: Fix memory leak when xhci_disable_slot() returns error usb: xhci: Return error when host is dead in xhci_disable_slot() usb: xhci: Remove xhci->mutex from xhci_alloc_dev() usb: xhci: Handle USB transaction error on address command drivers/usb/host/xhci-hub.c | 5 +++- drivers/usb/host/xhci.c | 61 ++--- drivers/usb/host/xhci.h | 3 +-- 3 files changed, 29 insertions(+), 40 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/6] usb: xhci: Disable slot even virt-dev is null
xhci_disable_slot() is a helper for disabling a slot when a device
goes away or recovers from error situations. Currently, it checks
the corespoding virt-dev pointer and returns directly (w/o issuing
disable slot command) if it's null.
This is unnecessary and will cause problems in case where virt-dev
allocation fails and xhci_disable_slot() is called to roll back the
hardware state. Refer to the implementation of xhci_alloc_dev().
This patch removes lines to check virt-dev in xhci_disable_slot().
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-hub.c | 3 +++
drivers/usb/host/xhci.c | 4
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index ad89a6d..f0ae9df 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -612,6 +612,9 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
xhci_dbg(xhci, "Disable all slots\n");
spin_unlock_irqrestore(&xhci->lock, *flags);
for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
+ if (!xhci->devs[i])
+ continue;
+
retval = xhci_disable_slot(xhci, NULL, i);
if (retval)
xhci_err(xhci, "Failed to disable slot %d, %d. Enter
test mode anyway\n",
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 8767238..796c902 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3582,11 +3582,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct
xhci_command *command,
unsigned long flags;
u32 state;
int ret = 0;
- struct xhci_virt_device *virt_dev;
- virt_dev = xhci->devs[slot_id];
- if (!virt_dev)
- return -EINVAL;
if (!command)
command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
if (!command)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 6/6] usb: xhci: Handle USB transaction error on address command
Xhci driver handles USB transaction errors on transfer events, but transaction errors are possible on address device command completion events as well. The xHCI specification (section 4.6.5) says: A USB Transaction Error Completion Code for an Address Device Command may be due to a Stall response from a device. Software should issue a Disable Slot Command for the Device Slot then an Enable Slot Command to recover from this error. This patch handles USB transaction errors on address command completion events. The related discussion threads can be found through below links. http://marc.info/?l=linux-usb&m=149362010728921&w=2 http://marc.info/?l=linux-usb&m=149252752825755&w=2 Suggested-by: Mathias Nyman Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 311cded..e78bee8 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3835,8 +3835,14 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, break; case COMP_USB_TRANSACTION_ERROR: dev_warn(&udev->dev, "Device not responding to setup %s.\n", act); - ret = -EPROTO; - break; + + mutex_unlock(&xhci->mutex); + ret = xhci_disable_slot(xhci, udev->slot_id); + if (!ret) + xhci_alloc_dev(hcd, udev); + kfree(command->completion); + kfree(command); + return -EPROTO; case COMP_INCOMPATIBLE_DEVICE_ERROR: dev_warn(&udev->dev, "ERROR: Incompatible device for setup %s command\n", act); -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 5/6] usb: xhci: Remove xhci->mutex from xhci_alloc_dev()
xhci->mutex was added in xhci_alloc_dev() to protect two race sources
(xhci->slot_id and xhci->addr_dev) by commit a00918d0521d ("usb: host:
xhci: add mutex for non-thread-safe data").
While xhci->slot_id has been discarded in commit c2d3d49bba08 ("usb:
xhci: move slot_id from xhci_hcd to xhci_command structure"), and
xhci->addr_dev has been removed in commit 87e44f2aac8d ("usb: xhci:
remove the use of xhci->addr_dev"), it's now safe to remove the use of
xhci->mutex in xhci_alloc_dev().
Link: https://marc.info/?l=linux-usb&m=150306294725821&w=2
Suggested-by: Mathias Nyman
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 3bb3075..311cded 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3640,13 +3640,10 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct
usb_device *udev)
if (!command)
return 0;
- /* xhci->slot_id and xhci->addr_dev are not thread-safe */
- mutex_lock(&xhci->mutex);
spin_lock_irqsave(&xhci->lock, flags);
ret = xhci_queue_slot_control(xhci, command, TRB_ENABLE_SLOT, 0);
if (ret) {
spin_unlock_irqrestore(&xhci->lock, flags);
- mutex_unlock(&xhci->mutex);
xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
xhci_free_command(xhci, command);
return 0;
@@ -3656,7 +3653,6 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device
*udev)
wait_for_completion(command->completion);
slot_id = command->slot_id;
- mutex_unlock(&xhci->mutex);
if (!slot_id || command->status != COMP_SUCCESS) {
xhci_err(xhci, "Error while assigning device slot ID\n");
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 4/6] usb: xhci: Return error when host is dead in xhci_disable_slot()
xhci_disable_slot() is a helper for disabling a slot when a device
goes away or recovers from error situations. Currently, it returns
success when it sees a dead host. This is not the right way to go.
It should return error and let the invoker know that disable slot
command was failed due to a dead host.
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 1290674..3bb3075 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3583,10 +3583,9 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
state = readl(&xhci->op_regs->status);
if (state == 0x || (xhci->xhc_state & XHCI_STATE_DYING) ||
(xhci->xhc_state & XHCI_STATE_HALTED)) {
- xhci_free_virt_device(xhci, slot_id);
spin_unlock_irqrestore(&xhci->lock, flags);
kfree(command);
- return ret;
+ return -ENODEV;
}
ret = xhci_queue_slot_control(xhci, command, TRB_DISABLE_SLOT,
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/6] usb: xhci: Fix potential memory leak in xhci_disable_slot()
xhci_disable_slot() allows the invoker to pass a command pointer
as paramenter. Otherwise, it will allocate one. This will cause
memory leak when a command structure was allocated inside of this
function while queuing command trb fails. Another problem comes up
when the invoker passed a command pointer, but xhci_disable_slot()
frees it when it detects a dead host.
This patch fixes these two problems by removing the command parameter
from xhci_disable_slot().
Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
Cc: Guoqing Zhang
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-hub.c | 2 +-
drivers/usb/host/xhci.c | 30 +-
drivers/usb/host/xhci.h | 3 +--
3 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index f0ae9df..e35903d 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -615,7 +615,7 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
if (!xhci->devs[i])
continue;
- retval = xhci_disable_slot(xhci, NULL, i);
+ retval = xhci_disable_slot(xhci, i);
if (retval)
xhci_err(xhci, "Failed to disable slot %d, %d. Enter
test mode anyway\n",
i, retval);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 796c902..502842b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3532,14 +3532,9 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
struct xhci_virt_device *virt_dev;
struct xhci_slot_ctx *slot_ctx;
int i, ret;
- struct xhci_command *command;
xhci_debugfs_remove_slot(xhci, udev->slot_id);
- command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
- if (!command)
- return;
-
#ifndef CONFIG_USB_DEFAULT_PERSIST
/*
* We called pm_runtime_get_noresume when the device was attached.
@@ -3554,10 +3549,8 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
/* If the host is halted due to driver unload, we still need to free the
* device.
*/
- if (ret <= 0 && ret != -ENODEV) {
- kfree(command);
+ if (ret <= 0 && ret != -ENODEV)
return;
- }
virt_dev = xhci->devs[udev->slot_id];
slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx);
@@ -3569,22 +3562,21 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
del_timer_sync(&virt_dev->eps[i].stop_cmd_timer);
}
- xhci_disable_slot(xhci, command, udev->slot_id);
+ xhci_disable_slot(xhci, udev->slot_id);
/*
* Event command completion handler will free any data structures
* associated with the slot. XXX Can free sleep?
*/
}
-int xhci_disable_slot(struct xhci_hcd *xhci, struct xhci_command *command,
- u32 slot_id)
+int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
{
+ struct xhci_command *command;
unsigned long flags;
u32 state;
int ret = 0;
- if (!command)
- command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
+ command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
if (!command)
return -ENOMEM;
@@ -3603,7 +3595,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct
xhci_command *command,
slot_id);
if (ret) {
spin_unlock_irqrestore(&xhci->lock, flags);
- xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
+ kfree(command);
return ret;
}
xhci_ring_cmd_db(xhci);
@@ -3678,6 +3670,8 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device
*udev)
return 0;
}
+ xhci_free_command(xhci, command);
+
if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK)) {
spin_lock_irqsave(&xhci->lock, flags);
ret = xhci_reserve_host_control_ep_resources(xhci);
@@ -3715,18 +3709,12 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct
usb_device *udev)
pm_runtime_get_noresume(hcd->self.controller);
#endif
-
- xhci_free_command(xhci, command);
/* Is this a LS or FS device under a HS hub? */
/* Hub or peripherial? */
return 1;
disable_slot:
- /* Disable slot, if we can do it without mem alloc */
- kfree(command->completion);
- command->completion = NULL;
- command->status = 0;
- return xhci_disable_slot(xhci, command, udev->slot_id);
+ return xhci_disable_slot(xhci, udev->slot_id);
}
/*
diff --git a/drivers/usb/h
[PATCH v3 1/3] usb: xhci: Make some static functions global
This patch makes some static functions global to avoid duplications
in different files. These functions can be used in the implementation
of xHCI debug capability. There is no functional change.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-mem.c | 94 ++--
drivers/usb/host/xhci-ring.c | 4 +-
drivers/usb/host/xhci.h | 16 +++-
3 files changed, 72 insertions(+), 42 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 2a82c92..1ccb1c5 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -368,7 +368,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd
*xhci,
* Set the end flag and the cycle toggle bit on the last segment.
* See section 4.9.1 and figures 15 and 16.
*/
-static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
+struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
unsigned int num_segs, unsigned int cycle_state,
enum xhci_ring_type type, unsigned int max_packet, gfp_t flags)
{
@@ -467,7 +467,7 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct
xhci_ring *ring,
#define CTX_SIZE(_hcc) (HCC_64BYTE_CONTEXT(_hcc) ? 64 : 32)
-static struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd
*xhci,
+struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd *xhci,
int type, gfp_t flags)
{
struct xhci_container_ctx *ctx;
@@ -492,7 +492,7 @@ static struct xhci_container_ctx
*xhci_alloc_container_ctx(struct xhci_hcd *xhci
return ctx;
}
-static void xhci_free_container_ctx(struct xhci_hcd *xhci,
+void xhci_free_container_ctx(struct xhci_hcd *xhci,
struct xhci_container_ctx *ctx)
{
if (!ctx)
@@ -1762,21 +1762,61 @@ void xhci_free_command(struct xhci_hcd *xhci,
kfree(command);
}
+int xhci_alloc_erst(struct xhci_hcd *xhci,
+ struct xhci_ring *evt_ring,
+ struct xhci_erst *erst,
+ gfp_t flags)
+{
+ size_t size;
+ unsigned int val;
+ struct xhci_segment *seg;
+ struct xhci_erst_entry *entry;
+
+ size = sizeof(struct xhci_erst_entry) * evt_ring->num_segs;
+ erst->entries = dma_alloc_coherent(xhci_to_hcd(xhci)->self.sysdev,
+ size,
+ &erst->erst_dma_addr,
+ flags);
+ if (!erst->entries)
+ return -ENOMEM;
+
+ memset(erst->entries, 0, size);
+ erst->num_entries = evt_ring->num_segs;
+
+ seg = evt_ring->first_seg;
+ for (val = 0; val < evt_ring->num_segs; val++) {
+ entry = &erst->entries[val];
+ entry->seg_addr = cpu_to_le64(seg->dma);
+ entry->seg_size = cpu_to_le32(TRBS_PER_SEGMENT);
+ entry->rsvd = 0;
+ seg = seg->next;
+ }
+
+ return 0;
+}
+
+void xhci_free_erst(struct xhci_hcd *xhci, struct xhci_erst *erst)
+{
+ size_t size;
+ struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
+
+ size = sizeof(struct xhci_erst_entry) * (erst->num_entries);
+ if (erst->entries)
+ dma_free_coherent(dev, size,
+ erst->entries,
+ erst->erst_dma_addr);
+ erst->entries = NULL;
+}
+
void xhci_mem_cleanup(struct xhci_hcd *xhci)
{
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
- int size;
int i, j, num_ports;
cancel_delayed_work_sync(&xhci->cmd_timer);
- /* Free the Event Ring Segment Table and the actual Event Ring */
- size = sizeof(struct xhci_erst_entry)*(xhci->erst.num_entries);
- if (xhci->erst.entries)
- dma_free_coherent(dev, size,
- xhci->erst.entries, xhci->erst.erst_dma_addr);
- xhci->erst.entries = NULL;
- xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed ERST");
+ xhci_free_erst(xhci, &xhci->erst);
+
if (xhci->event_ring)
xhci_ring_free(xhci, xhci->event_ring);
xhci->event_ring = NULL;
@@ -2313,9 +2353,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
unsigned intval, val2;
u64 val_64;
- struct xhci_segment *seg;
- u32 page_size, temp;
- int i;
+ u32 page_size, temp;
+ int i, ret;
INIT_LIST_HEAD(&xhci->cmd_list);
@@ -2454,32 +2493,9 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
if (xhci_check_trb_in_td_math(xhci) < 0)
goto fail;
- xhci->erst.entries =
[PATCH v3 3/3] usb: doc: Update document for USB3 debug port usage
Update Documentation/driver-api/usb/usb3-debug-port.rst. This update includes the guide for using xHCI debug capability based TTY serial link. Signed-off-by: Lu Baolu --- Documentation/driver-api/usb/usb3-debug-port.rst | 68 1 file changed, 68 insertions(+) diff --git a/Documentation/driver-api/usb/usb3-debug-port.rst b/Documentation/driver-api/usb/usb3-debug-port.rst index feb1a36..4ad4787 100644 --- a/Documentation/driver-api/usb/usb3-debug-port.rst +++ b/Documentation/driver-api/usb/usb3-debug-port.rst @@ -98,3 +98,71 @@ you to check the sanity of the setup. cat /dev/ttyUSB0 done = end of bash scripts === + +Serial TTY +== + +DbC has also been designed for a serial TTY device at runtime. +One use of this is running a login service on the debug target. +Hence it can be remote accessed by the debug host. Another use +can probably be found in servers. It provides a peer-to-peer USB +link between two host-only machines. This provides a reasonable +out-of-band communication method between two servers. + +In order to use this, you need to make sure your kernel has been +configured to support USB_XHCI_DBGCAP. A sysfs attribute under +the xHCI device node is used to enable or disable DbC. By default, +DbC is disabled:: + + root@target:/sys/bus/pci/devices/:00:14.0# cat dbc + disabled + +Enable DbC with the following command:: + + root@target:/sys/bus/pci/devices/:00:14.0# echo enable > dbc + +You can check the DbC state at anytime:: + + root@target:/sys/bus/pci/devices/:00:14.0# cat dbc + enabled + +Connect the debug target to the debug host with a USB 3.0 super- +speed A-to-A debugging cable. You can see the /dev/ttyDBC0 created +on the debug target. You will see below kernel message lines:: + + root@target: tail -f /var/log/kern.log + [ 182.730103] xhci_hcd :00:14.0: DbC connected + [ 191.169420] xhci_hcd :00:14.0: DbC configured + [ 191.169597] xhci_hcd :00:14.0: DbC now attached to /dev/ttyDBC0 + +Accordingly, the DbC state has been brought up to:: + + root@host:/sys/bus/pci/devices/:00:14.0# cat dbc + configured + +On the debug host, you will see the debug device has been enumerated. +You will see below kernel message lines:: + + root@host: tail -f /var/log/kern.log + [ 79.454780] usb 2-2.1: new SuperSpeed USB device number 3 using xhci_hcd + [ 79.475003] usb 2-2.1: LPM exit latency is zeroed, disabling LPM. + [ 79.475389] usb 2-2.1: New USB device found, idVendor=1d6b, idProduct=0004 + [ 79.475390] usb 2-2.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 + [ 79.475391] usb 2-2.1: Product: Remote GDB + [ 79.475392] usb 2-2.1: Manufacturer: Linux + [ 79.475393] usb 2-2.1: SerialNumber: 0001 + [ 79.660368] usb_debug 2-2.1:1.0: xhci_dbc converter detected + [ 79.660439] usb 2-2.1: xhci_dbc converter now attached to ttyUSB0 + +You can simply verify whether it works by:: + + # On target side + root@target: echo "Hello world" > /dev/ttyDBC0 + + # On host side + root@host: cat /dev/ttyUSB0 + Hello world + + # vice versa + +You have a workable serial link over USB now. -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver
xHCI compatible USB host controllers(i.e. super-speed USB3 controllers) can be implemented with the Debug Capability(DbC). It presents a debug device which is fully compliant with the USB framework and provides the equivalent of a very high performance full-duplex serial link. The debug capability operation model and registers interface are defined in 7.6.8 of the xHCI specification, revision 1.1. The DbC debug device shares a root port with the xHCI host. By default, the debug capability is disabled and the root port is assigned to xHCI. When the DbC is enabled, the root port will be assigned to the DbC debug device, and the xHCI sees nothing on this port. This implementation uses a sysfs node named under the xHCI device to manage the enabling and disabling of the debug capability. When the debug capability is enabled, it will present a debug device through the debug port. This debug device is fully compliant with the USB3 framework, and it can be enumerated by a debug host on the other end of the USB link. As soon as the debug device is configured, a TTY serial device named /dev/ttyDBC0 will be created. One use of this link is running a login service on the debug target. Hence it can be remote accessed by a debug host. Another use case can probably be found in servers. It provides a peer-to-peer USB link between two host-only machines. This provides a reasonable out-of-band communication method between two servers. Signed-off-by: Lu Baolu --- .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd | 25 + drivers/usb/host/Kconfig |9 + drivers/usb/host/Makefile |5 + drivers/usb/host/xhci-dbgcap.c | 1016 drivers/usb/host/xhci-dbgcap.h | 247 + drivers/usb/host/xhci-dbgtty.c | 586 +++ drivers/usb/host/xhci-trace.h | 60 ++ drivers/usb/host/xhci.c| 10 + drivers/usb/host/xhci.h|1 + 9 files changed, 1959 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd create mode 100644 drivers/usb/host/xhci-dbgcap.c create mode 100644 drivers/usb/host/xhci-dbgcap.h create mode 100644 drivers/usb/host/xhci-dbgtty.c diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd new file mode 100644 index 000..0088aba --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd @@ -0,0 +1,25 @@ +What: /sys/bus/pci/drivers/xhci_hcd/.../dbc +Date: June 2017 +Contact: Lu Baolu +Description: + xHCI compatible USB host controllers (i.e. super-speed + USB3 controllers) are often implemented with the Debug + Capability (DbC). It can present a debug device which + is fully compliant with the USB framework and provides + the equivalent of a very high performance full-duplex + serial link for debug purpose. + + The DbC debug device shares a root port with xHCI host. + When the DbC is enabled, the root port will be assigned + to the Debug Capability. Otherwise, it will be assigned + to xHCI. + + Writing "enable" to this attribute will enable the DbC + functionality and the shared root port will be assigned + to the DbC device. Writing "disable" to this attribute + will disable the DbC functionality and the shared root + port will roll back to the xHCI. + + Reading this attribute gives the state of the DbC. It + can be one of the following states: disabled, enabled, + initialized, connected, configured and stalled. diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index fa5692d..968a196 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -27,6 +27,15 @@ config USB_XHCI_HCD module will be called xhci-hcd. if USB_XHCI_HCD +config USB_XHCI_DBGCAP + bool "xHCI support for debug capability" + depends on TTY + select USB_U_SERIAL + ---help--- + Say 'Y' to enable the support for the xHCI debug capability. Make + sure that your xHCI host supports the extended debug capability and + you want a TTY serial device based on the xHCI debug capability + before enabling this option. If unsure, say 'N'. config USB_XHCI_PCI tristate diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile index cf2691f..175c80a 100644 --- a/drivers/usb/host/Makefile +++ b/drivers/usb/host/Makefile @@ -13,6 +13,11 @@ fhci-$(CONFIG_FHCI_DEBUG) += fhci-dbg.o xhci-hcd-y := xhci.o xhci-mem.o xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o xhci-hcd-y
[PATCH v3 0/3] usb: xhci: Add debug capability support in xhci
Hi, This series is for xHCI debug capability (spec section 7.6.8) support in the xHCI driver. xHCI compatible USB host controllers(i.e. super-speed USB3 controllers) can be implemented with the Debug Capability(DbC). It presents a debug device which is fully compliant with the USB framework and provides the equivalent of a very high performance full-duplex serial link. The debug capability operation model and registers interface are defined in 7.6.8 of the xHCI specification, revision 1.1. The DbC debug device shares a root port with the xHCI host. By default, the debug capability is disabled and the root port is assigned to xHCI. When the DbC is enabled, the root port will be assigned to the DbC debug device, and the xHCI sees nothing on this port. This implementation uses a sysfs node named under the xHCI device to manage the enabling and disabling of the debug capability. When the debug capability is enabled, it will present a debug device through the debug port. This debug device is fully compliant with the USB3 framework, and it can be enumerated by a debug host on the other end of the USB link. As soon as the debug device is configured, a TTY serial device named /dev/ttyGSn will be created. One use of this link is running a login service on the debug target. Hence it can be remote accessed by a debug host. Another use case can probably be found in servers. It provides a peer-to-peer USB link between two host-only machines. This provides a reasonable out-of-band communication method between two servers. Best regards, Lu Baolu --- Change log: v1->v2: - Add a new patch to move u_serial.c from drivers/usb/gadget/function to drivers/usb/common/ and move u_serial.h to include/linux/usb/. v2->v3: - Remove the use of u_serial and add a new tty glue for debug capability. Lu Baolu (3): usb: xhci: Make some static functions global usb: xhci: Add DbC support in xHCI driver usb: doc: Update document for USB3 debug port usage .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd | 25 + Documentation/driver-api/usb/usb3-debug-port.rst | 68 ++ drivers/usb/host/Kconfig |9 + drivers/usb/host/Makefile |5 + drivers/usb/host/xhci-dbgcap.c | 1016 drivers/usb/host/xhci-dbgcap.h | 247 + drivers/usb/host/xhci-dbgtty.c | 586 +++ drivers/usb/host/xhci-mem.c| 94 +- drivers/usb/host/xhci-ring.c |4 +- drivers/usb/host/xhci-trace.h | 60 ++ drivers/usb/host/xhci.c| 10 + drivers/usb/host/xhci.h| 17 +- 12 files changed, 2099 insertions(+), 42 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd create mode 100644 drivers/usb/host/xhci-dbgcap.c create mode 100644 drivers/usb/host/xhci-dbgcap.h create mode 100644 drivers/usb/host/xhci-dbgtty.c -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] usb: xhci: use list_is_singular for cmd_list
Use list_is_singular() to check if cmd_list has only one entry.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 95be9e8..40947e7 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1418,7 +1418,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
}
/* restart timer if this wasn't the last command */
- if (cmd->cmd_list.next != &xhci->cmd_list) {
+ if (!list_is_singular(&xhci->cmd_list)) {
xhci->current_cmd = list_entry(cmd->cmd_list.next,
struct xhci_command, cmd_list);
mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
@@ -3787,7 +3787,7 @@ static int queue_command(struct xhci_hcd *xhci, struct
xhci_command *cmd,
list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
/* if there are no other commands queued we start the timeout timer */
- if (xhci->cmd_list.next == &cmd->cmd_list &&
+ if (list_is_singular(&xhci->cmd_list) &&
!timer_pending(&xhci->cmd_timer)) {
xhci->current_cmd = cmd;
mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] usb: xhci: remove unnecessary return in xhci_pci_setup()
Remove the unnecessary return line in xhci_pci_setup(). Signed-off-by: Lu Baolu --- drivers/usb/host/xhci-pci.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index e96ae80..ace3f57 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -242,8 +242,6 @@ static int xhci_pci_setup(struct usb_hcd *hcd) /* Find any debug ports */ retval = xhci_pci_reinit(xhci, pdev); - if (!retval) - return retval; return retval; } -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] several cleanup patches
Hi Mathias, This series includes several tiny cleanup patches for xhci host controller driver. These make the code slightly more readable. There's no functional change. Please consider it for your for-usb-next branch. Best regards, Lu Baolu Lu Baolu (4): usb: xhci: remove unnecessary assignment usb: xhci: avoid unnecessary calculation usb: xhci: use list_is_singular for cmd_list usb: xhci: remove unnecessary return in xhci_pci_setup() drivers/usb/host/xhci-pci.c | 2 -- drivers/usb/host/xhci-ring.c | 26 -- 2 files changed, 12 insertions(+), 16 deletions(-) -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] usb: xhci: remove unnecessary assignment
Drop an unnecessary assignment in prepare_transfer(). Signed-off-by: Lu Baolu --- drivers/usb/host/xhci-ring.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index bdf6b13..3cc303f 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -2826,8 +2826,6 @@ static int prepare_transfer(struct xhci_hcd *xhci, td->start_seg = ep_ring->enq_seg; td->first_trb = ep_ring->enqueue; - urb_priv->td[td_index] = td; - return 0; } -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] usb: xhci: avoid unnecessary calculation
No need to calculate remainder and length_field, if there is
no data phase of a control transfer.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 3cc303f..95be9e8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3219,7 +3219,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
struct usb_ctrlrequest *setup;
struct xhci_generic_trb *start_trb;
int start_cycle;
- u32 field, length_field, remainder;
+ u32 field;
struct urb_priv *urb_priv;
struct xhci_td *td;
@@ -3292,16 +3292,16 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
else
field = TRB_TYPE(TRB_DATA);
- remainder = xhci_td_remainder(xhci, 0,
- urb->transfer_buffer_length,
- urb->transfer_buffer_length,
- urb, 1);
-
- length_field = TRB_LEN(urb->transfer_buffer_length) |
- TRB_TD_SIZE(remainder) |
- TRB_INTR_TARGET(0);
-
if (urb->transfer_buffer_length > 0) {
+ u32 length_field, remainder;
+
+ remainder = xhci_td_remainder(xhci, 0,
+ urb->transfer_buffer_length,
+ urb->transfer_buffer_length,
+ urb, 1);
+ length_field = TRB_LEN(urb->transfer_buffer_length) |
+ TRB_TD_SIZE(remainder) |
+ TRB_INTR_TARGET(0);
if (setup->bRequestType & USB_DIR_IN)
field |= TRB_DIR_IN;
queue_trb(xhci, ep_ring, true,
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] usb: xhci: remove unnecessary return in xhci_pci_setup()
Hi, On 01/05/2017 02:12 AM, Andy Shevchenko wrote: > On Wed, Jan 4, 2017 at 3:51 AM, Lu Baolu wrote: >> Remove the unnecessary return line in xhci_pci_setup(). >> >> Signed-off-by: Lu Baolu >> --- >> drivers/usb/host/xhci-pci.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c >> index e96ae80..ace3f57 100644 >> --- a/drivers/usb/host/xhci-pci.c >> +++ b/drivers/usb/host/xhci-pci.c >> @@ -242,8 +242,6 @@ static int xhci_pci_setup(struct usb_hcd *hcd) >> >> /* Find any debug ports */ >> retval = xhci_pci_reinit(xhci, pdev); >> - if (!retval) >> - return retval; >> >> return retval; > Then better just > > return xhci_pci_reinit(xhci, pdev); > Fair enough. Thanks. Best regards, Lu Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] usb: xhci: set RWE only for remote wakeup capable devices
Xhci spec requires in section 4.23.5.1.1.1 that the RWE bit of USB2 PORTPMSC register should only set for remote wakeup capble devices. This was suggested by Mathias in the following discussion thread. http://marc.info/?l=linux-usb&m=148154757829677&w=2 Suggested-by: Mathias Nyman Signed-off-by: Lu Baolu --- drivers/usb/host/xhci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 0c8deb9..d887e09 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -4191,7 +4191,9 @@ int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd, } pm_val &= ~PORT_HIRD_MASK; - pm_val |= PORT_HIRD(hird) | PORT_RWE | PORT_L1DS(udev->slot_id); + pm_val |= PORT_HIRD(hird) | PORT_L1DS(udev->slot_id); + if (udev->do_remote_wakeup) + pm_val |= PORT_RWE; writel(pm_val, pm_addr); pm_val = readl(pm_addr); pm_val |= PORT_HLE; -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] usb: xhci: set RWE only for remote wakeup capable devices
Hi Mathias, On 01/12/2017 04:53 PM, Lu Baolu wrote: > Xhci spec requires in section 4.23.5.1.1.1 that the RWE bit of USB2 > PORTPMSC register should only set for remote wakeup capble devices. > > This was suggested by Mathias in the following discussion thread. > http://marc.info/?l=linux-usb&m=148154757829677&w=2 > > Suggested-by: Mathias Nyman > Signed-off-by: Lu Baolu > --- > drivers/usb/host/xhci.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 0c8deb9..d887e09 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -4191,7 +4191,9 @@ int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd, > } > > pm_val &= ~PORT_HIRD_MASK; > - pm_val |= PORT_HIRD(hird) | PORT_RWE | PORT_L1DS(udev->slot_id); > + pm_val |= PORT_HIRD(hird) | PORT_L1DS(udev->slot_id); > + if (udev->do_remote_wakeup) This is not the right thing to check remote wakeup capability of a device. I will rework it and submit v2. Please just ignore this one. Sorry about this. Best regards, Lu Baolu > + pm_val |= PORT_RWE; > writel(pm_val, pm_addr); > pm_val = readl(pm_addr); > pm_val |= PORT_HLE; -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 30/37] usb: host: xhci: make a generic TRB tracer
IDT ? 'I' : 'i', > + field3 & TRB_IOC ? 'I' : 'i', > + field3 & TRB_CHAIN ? 'C' : 'c', > + field3 & TRB_NO_SNOOP ? 'S' : 's', > + field3 & TRB_ISP ? 'I' : 'i', > + field3 & TRB_ENT ? 'E' : 'e', > + field3 & TRB_CYCLE ? 'C' : 'c'); > + break; > + case TRB_NORMAL: > + case TRB_DATA: > + case TRB_STATUS: > + case TRB_ISOC: > + case TRB_EVENT_DATA: > + case TRB_TR_NOOP: > + sprintf(str, "Buffer %08x%08x length %d TD size %d intr %d type > '%s' flags %c:%c:%c:%c:%c:%c:%c:%c", > + field1, field0, TRB_LEN(field2), > GET_TD_SIZE(field2), > + GET_INTR_TARGET(field2), > + xhci_trb_type_string(TRB_FIELD_TO_TYPE(field3)), > + field3 & TRB_BEI ? 'B' : 'b', > + field3 & TRB_IDT ? 'I' : 'i', > + field3 & TRB_IOC ? 'I' : 'i', > + field3 & TRB_CHAIN ? 'C' : 'c', > + field3 & TRB_NO_SNOOP ? 'S' : 's', > + field3 & TRB_ISP ? 'I' : 'i', > + field3 & TRB_ENT ? 'E' : 'e', > + field3 & TRB_CYCLE ? 'C' : 'c'); > + break; > + > + case TRB_CMD_NOOP: > + case TRB_ENABLE_SLOT: > + sprintf(str, "%s: flags %c", > + xhci_trb_type_string(TRB_FIELD_TO_TYPE(field3)), > + field3 & TRB_CYCLE ? 'C' : 'c'); > + break; > + case TRB_DISABLE_SLOT: > + case TRB_NEG_BANDWIDTH: > + sprintf(str, "%s: slot %d flags %c", > + xhci_trb_type_string(TRB_FIELD_TO_TYPE(field3)), > + TRB_TO_SLOT_ID(field3), > + field3 & TRB_CYCLE ? 'C' : 'c'); > + break; > + case TRB_ADDR_DEV: > + sprintf(str, "%s: ctx %08x%08x slot %d flags %c:%c", > + xhci_trb_type_string(TRB_FIELD_TO_TYPE(field3)), > + field1, field0, > + TRB_TO_SLOT_ID(field3), > + field3 & TRB_BSR ? 'B' : 'b', > + field3 & TRB_CYCLE ? 'C' : 'c'); > + break; > + case TRB_CONFIG_EP: > + sprintf(str, "%s: ctx %08x%08x slot %d flags %c:%c", > + xhci_trb_type_string(TRB_FIELD_TO_TYPE(field3)), > + field1, field0, > + TRB_TO_SLOT_ID(field3), > + field3 & TRB_DC ? 'D' : 'd', > + field3 & TRB_CYCLE ? 'C' : 'c'); > + break; > + case TRB_EVAL_CONTEXT: > + sprintf(str, "%s: ctx %08x%08x slot %d flags %c", > + xhci_trb_type_string(TRB_FIELD_TO_TYPE(field3)), > + field1, field0, > + TRB_TO_SLOT_ID(field3), > + field3 & TRB_CYCLE ? 'C' : 'c'); > + break; > + case TRB_RESET_EP: > + sprintf(str, "%s: ctx %08x%08x slot %d ep %d flags %c", > + xhci_trb_type_string(TRB_FIELD_TO_TYPE(field3)), > + field1, field0, > + TRB_TO_SLOT_ID(field3), > + /* Macro decrements 1, maybe it shouldn't?!? */ > + TRB_TO_EP_INDEX(field3) + 1, > + field3 & TRB_CYCLE ? 'C' : 'c'); > + break; > + case TRB_STOP_RING: > + sprintf(str, "%s: slot %d sp %d ep %d flags %c", > + xhci_trb_type_string(TRB_FIELD_TO_TYPE(field3)), > + TRB_TO_SLOT_ID(field3), > + TRB_TO_SUSPEND_PORT(field3), >
Re: [PATCH 31/37] usb: host: xhci: add urb_enqueue/dequeue/giveback tracers
+ TP_ARGS(urb)
> +);
> +
> +DEFINE_EVENT(xhci_log_urb, xhci_urb_dequeue,
> + TP_PROTO(struct urb *urb),
> + TP_ARGS(urb)
> +);
> +
> #endif /* __XHCI_TRACE_H */
>
> /* this part must be outside header guard */
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 06d294d7e01f..c0f3670e5a51 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1383,6 +1383,8 @@ int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb
> *urb, gfp_t mem_flags)
> urb_priv->td_cnt = 0;
> urb->hcpriv = urb_priv;
>
> + trace_xhci_urb_enqueue(urb);
> +
> if (usb_endpoint_xfer_control(&urb->ep->desc)) {
> /* Check to see if the max packet size for the default control
>* endpoint changed during FS device enumeration
> @@ -1509,6 +1511,9 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb
> *urb, int status)
>
> xhci = hcd_to_xhci(hcd);
> spin_lock_irqsave(&xhci->lock, flags);
> +
> + trace_xhci_urb_dequeue(urb);
> +
> /* Make sure the URB hasn't completed or been unlinked already */
> ret = usb_hcd_check_unlink_urb(hcd, urb, status);
> if (ret || !urb->hcpriv)
Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 30/37] usb: host: xhci: make a generic TRB tracer
Hi,
On 01/16/2017 03:03 PM, Felipe Balbi wrote:
> Hi,
>
> Lu Baolu writes:
>> On 12/29/2016 07:01 PM, Felipe Balbi wrote:
>>> instead of having a tracer that can only trace command completions,
>>> let's promote this tracer so it can trace and decode any TRB.
>>>
>>> With that, it will be easier to extrapolate the lifetime of any TRB
>>> which might help debugging certain issues.
>>>
>>> Signed-off-by: Felipe Balbi
>>> ---
>>> drivers/usb/host/xhci-ring.c | 14 +-
>>> drivers/usb/host/xhci-trace.h | 55 +---
>>> drivers/usb/host/xhci.h | 311
>>> ++
>>> 3 files changed, 357 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>>> index 393c64f8acef..0ee7d358b812 100644
>>> --- a/drivers/usb/host/xhci-ring.c
>>> +++ b/drivers/usb/host/xhci-ring.c
>>> @@ -1346,6 +1346,9 @@ static void handle_cmd_completion(struct xhci_hcd
>>> *xhci,
>>>
>>> cmd_dma = le64_to_cpu(event->cmd_trb);
>>> cmd_trb = xhci->cmd_ring->dequeue;
>>> +
>>> + trace_xhci_handle_command(xhci->cmd_ring, &cmd_trb->generic);
>>> +
>> This is duplicated with trace_xhci_handle_event().
> no it's not. They separate events from the same event class, but they're
> different things.
Comments below.
>
>>> @@ -2407,6 +2408,10 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>>>
>>> ep_trb = &ep_seg->trbs[(ep_trb_dma - ep_seg->dma) /
>>> sizeof(*ep_trb)];
>>> +
>>> + trace_xhci_handle_transfer(ep_ring,
>>> + (struct xhci_generic_trb *) ep_trb);
>>> +
>> This is duplicated with trace_xhci_handle_event().
> ditto
>
>>> + __entry->type = ring->type;
>>> + __entry->field0 = le32_to_cpu(trb->field[0]);
>>> + __entry->field1 = le32_to_cpu(trb->field[1]);
>>> + __entry->field2 = le32_to_cpu(trb->field[2]);
>>> + __entry->field3 = le32_to_cpu(trb->field[3]);
>>> ),
>>> - TP_printk("\ntrb_dma=@%llx, trb_va=@%p, status=%08x, flags=%08x",
>>> - (unsigned long long) __entry->dma, __entry->va,
>>> - __entry->status, __entry->flags
>>> + TP_printk("%s: %s", xhci_ring_type_string(__entry->type),
>> How about moving the common fields of a TRB (like TRB type and
>> the cycle bit) to the place between ring type and trb decoding string?
>> And remove type and cycle decoding in xhci_decode_trb() as well.
>>
>> Something like:
>>
>> diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
>> index d01524b..f8ef7b8 100644
>> --- a/drivers/usb/host/xhci-trace.h
>> +++ b/drivers/usb/host/xhci-trace.h
>> @@ -132,9 +132,11 @@ DECLARE_EVENT_CLASS(xhci_log_trb,
>> __entry->field2 = le32_to_cpu(trb->field[2]);
>> __entry->field3 = le32_to_cpu(trb->field[3]);
>> ),
>> - TP_printk("%s: %s", xhci_ring_type_string(__entry->type),
>> - xhci_decode_trb(__entry->field0, __entry->field1,
>> - __entry->field2, __entry->field3)
>> + TP_printk("%s: %s: %c: %s", xhci_ring_type_string(__entry->type),
>> + xhci_trb_type_string(TRB_FIELD_TO_TYPE(__entry->field3)),
>> + __entry->field3 & TRB_CYCLE ? 'C' : 'c',
>> + xhci_decode_trb(__entry->field0, __entry->field1,
>> + __entry->field2, __entry->field3)
> and what do I get with that? What's the actual benefit?
I thought that it could make xhci_decode_trb() a bit simpler.
>
>> )
>> );
>>
>>
>>> + xhci_decode_trb(__entry->field0, __entry->field1,
>>> + __entry->field2, __entry->field3)
>>> )
>>> );
>>>
>>> -DEFINE_EVENT(xhci_log_event, xhci_cmd_completion,
>>> - TP_PROTO(void *trb_va, struct xhci_generic_trb *ev),
>>> - TP_ARGS(trb_va, ev)
>>> +DEFINE_EVENT(xhci_log_trb, xhci_handle_event,
>>> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
&
