Re: [RFC 2/3] EHCI: convert the IRQ handler to a tasklet
To begin with, the whole point of this RFC was to show that moving the entire IRQ handler (or even a large part of it) to a tasklet would have been at least as simple as moving the givebacks alone. Now that I realize the hrtimer and unlink pathways would have to be changed too, it's not quite so simple as it seemed at first. Still, I think it would be no worse than the work you did. It also is more typical of the way people expect the work to be divided between a top half and a bottom half -- usually the top half does little more than acknowledge the interrupt. Since the proposal was meant only as a proof of principle, I'm not going to develope it any farther. But I will respond to the points you raised in your review. On Sat, 24 Aug 2013, Ming Lei wrote: But the current interrupt from EHCI isn't be acked(USBSTS isn't cleared), so EHCI HW don't know the irq has been handled, then it is reasonable that the EHCI interrupt still interrupts CPU. EHCI spec(4.15) also states the point clearly: the host controller is not required to de-assert a currently active interrupt condition when software sets the interrupt enables (in the USBINR register, see Section 2.3.3) to a zero. The only reliable method software should use for acknowledging an interrupt is by transitioning the appropriate status bits in the USBSTS register (Section 2.3.2) from a one to a zero. Ooh, you're right! I had completely forgotten about this. It's a serious error in the RFC patch. Fixing it would be pretty easy, however. Not handling interrupts right away is the price we pay for using a bottom half. Your tasklet implementation isn't very different. Although the interrupt handler may realize quickly that a transfer has finished, there will still be a delay before the URB's completion handler can run. And the length of that delay will depend on the tasklet schedule delay. In my tasklet implementation, the next EHCI interrupt can be handled after the current hard interrupt is handled, but in this patch the EHCI hard irq can't be handled until the tasklet is handled, right? Right. But in the end, it doesn't matter. Remember, more than 99% of the work an HCD does is handling URBs. (HCDs do a couple of other things, such as detecting connections and disconnection or handling wakeups, but they are very small compared to the number of URBs it handles.) Consider also that an URB isn't done until it is given back to the driver that submitted it. Therefore, when measuring performance or latency of an HCD, what matters is how long the driver has to wait from URB submission to URB completion. If any part of that processing takes place in a tasklet, the tasklet delay will add in to the total time. It doesn't matter whether the tasklet handles only the giveback or some of the HCD's internal work as well. Thirdly, fatal error should have been handled immediately inside hard interrupt handler. Why? What difference does it make? At least, EHCI HW generates the fatal irq ASAP, This interrupt is not delayed to the next interrupt threshold.(4.15.2.4), so it is reasonable for software to handle it asap. That doesn't seem like a good reason. (Probably it just reflects the fact that once a fatal error has occurred, the controller hardware can't reliably determine when the next interrupt threshold will occur.) But of course, it would be easy to put that part of the code into the top half of the handler. Finally, tasklet schedule should have been optimized here if the tasklet is running on another CPU, otherwise the current CPU may spin on the tasklet lock until the same tasklet is completed on another CPU. That could be added in. But doesn't the core kernel's tasklet implementation already take care of this? The tasklet_hi_action() routine uses tasklet_trylock(), so it doesn't spin. OK, but extra tasklet schedule delay might be caused. Like I said, this feature also could be added easily. There's no point in enabling interrupts before they can be handled. As long as the tasklet is running, it doesn't do any good to receive more IRQs. Why doesn't any good to receive more IRWQs? And there should be other interrupts(non transfer complete irq, such as port change, fatal error, ...) comes. There's no reason to handle those events any more quickly than ordinary completion IRQs. Of course, the code _could_ be structured to leave interrupts unmasked while the tasklet runs. The tasklet would end up doing the same amount of work; the only difference would be that more interrupts occur while the tasklet runs, thereby wasting CPU time. To put it another way, which would you prefer: To have three interrupts while the tasklet is running, or one interrupt as soon as it finishes? Also if the latest value of irqs_are_masked isn't see by ehci_irq immediately, it will cause CPU interrupted by extra and
Re: [PATCH v1] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled
On Sat, 24 Aug 2013, Ming Lei wrote: For other HCDs, this changes on interrupt QH unlink may not need at all if there is no much cost about relink. But for some HCDs, it will be needed. Which HCDs need them? I haven't checked in detail. It seems likely that uhci-hcd and ohci-hcd will need changes. Are you sure? If the change is absolutely required, there is certainly bug in them, since HCDs still need to support submitting interrupt URB in tasklet scheduled by its complete() handler? Yes, I am sure, and no, there is no bug. Even for EHCI HCD, the interrupt QH unlink change isn't necessary, and the patch is only for improving the situation, isn't it? For other HCDs, basically unlink interrupt queue need't the wait time, so don't need the change. Wrong: For other HCDs, unlinks _do_ need to wait for the hardware. Just take a quick look at UHCI code, looks there is no much difference about unlinking qh between async qh and intr qh, which means the change might not need for uhci since usb-storage is common case to support. Correct me if it is wrong. The problem is that once an interrupt QH has been unlinked, relinking it might not make it visible to the hardware until the next frame starts. Therefore interrupt endpoints with an interval of 1 ms would not work correctly; they would get unlinked and relinked in every second frame. As I said, the only change introduced with running giveback in tasklet is that iso_stream_schedule() may see list_empty(stream-td_list) when underrun, and we can handle it inside HCD and usbcore, nothing to do with drivers. That's not true, at least, not for ehci-hcd. Could you explain it in detail? I mean we only discuss the change on isoc schedule in case of underrun when giveback in tasklet. The code in iso_stream_schedule() knows that any URB scheduled to end before ehci-last_iso_frame will already have been given back, and any resubmissions by the completion handler will have occurred already. That's what this comment means: /* * Use ehci-last_iso_frame as the base. There can't be any * TDs scheduled for earlier than that. */ But with your tasklet, this isn't true any more. Even though the driver has called usb_hcd_giveback_urb, the completion handler may not have run yet. OK, I give you a simple solution in which only one line change is needed for these HCDs, see attachment patch. The patch is wrong. It keeps track of whether the URB is being resubmitted by the completion handler, not whether the endpoint queue list_empty(stream-td_list) is already checked in iso_stream_schedule(), so I am wondering what is the 'wrong' you mean. Hmmm. For one thing, the patch doesn't include a definition of hcd_is_urb_completing(). For another, it won't work if the driver keeps a pool of URBs and shares them among multiple endpoints. We need to know if the empty endpoint queue is caused by the last completed URB in queue, which will be resubmitted later. has emptied out. What happens if the completion handler for URB0 submits URB1? Do you have such example? I posted a proposal for exactly this sort of thing a few weeks ago. The idea was that the URBs have to vary in size, depending on what the device is doing, so you can't predict in advance how much data a single URB will contain. But the driver wants to keep about the same amount of data on the output queue at all times. Therefore the completion handler will sometimes not submit any URBs, and sometimes it will submit more than one. It is possible for the two URBs which belong to different endpoint and let one of URBs to start the stream, but I am wondering it is reasonable that the two belong to same endpoint? Even for this case, we can handle it easily by checking if the two URBs belong to same endpoint. There's a much simpler way to solve this problem. I will post an RFC later on. To do it properly, you would need to count the number of URBs on the endpoint queue. The count would be decremented after the completion handler returned, which means it probably would have to be an atomic_t variable -- and I know that you hate those! percpu variable should be ok since no preemption is possible during completing? and no irq handler may operate the variable. Percpu variables are not always the best answer to everything! :-) Besides, even if your patch was correct, it still wouldn't be sufficient. ehci-hcd relies on the fact that the entries in a non-empty isochronous queue expire on or after ehci-last_iso_frame. With your patch, that wouldn't be true any more. Sorry, I don't understand your point, as I said, the only change on ISOC schedule introduced with running giveback in tasklet is that iso_stream_schedule() may see list_empty(stream-td_list) when underrun, and the above discussed patch should address this one.
Re: [PATCH v2 0/8] Common Clock Framework support for Samsung S3C64xx
On 08/20/13 16:09, Kukjin Kim wrote: Mike Turquette wrote: [...] OK, if new branch is ready, I will replace with that or if re-merge is required, I will. Either way, I'm fine and can handle. Mike, let me know your choice :-) Since I have already published it let's just go with the delta patch. I can create another stable branch named clk-next-s3c64xx-delta that just has this patch on top of clk-next-s3c64xx OR I can apply it on top of the existing clk-next-s3c64xx and re-merge it. Sounds good to me. If the branch for the delta is ready, let me know. Mike, I'm waiting for your delta branch which includes following from Tomasz. [PATCH] clk: samsung: pll: Use new registration method for PLL6552 and PLL6553 I couldn't send some branches to arm-soc for upcoming merge window yet because of build compilation breakage of common-clk-s3c64xx branch which has many dependencies... - Kukjin I'm trying to think on whether there are any weird git corner cases with re-merging clk-next-s3c64xx. Let me know if re-merging is somehow unsafe (makes history weird, or whatever). I don't think it causes some problem. Let me know what option is better for you. I'll publish as soon as I get the delta patch. Apologies again for creating some extra work! No problem. -- 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
[GIT PATCH] USB fixes for 3.11-rc7
The following changes since commit b36f4be3de1b123d8601de062e7dbfc904f305fb: Linux 3.11-rc6 (2013-08-18 14:36:53 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/ tags/usb-3.11-rc7 for you to fetch changes up to 52d5b9aba1f5790ca3231c262979c2c3e26dd99b: usb: phy: fix build breakage (2013-08-23 10:41:46 -0700) USB fixes for 3.11-rc7 Here are two USB fixes for 3.11-rc7 One fixes a reported regression in the OHCI driver, and the other fixes a reported build breakage in the USB phy drivers. Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org Alan Stern (1): USB: OHCI: add missing PCI PM callbacks to ohci-pci.c Anatolij Gustschin (1): usb: phy: fix build breakage drivers/usb/host/ohci-pci.c | 5 + drivers/usb/phy/phy-fsl-usb.h | 2 +- drivers/usb/phy/phy-fsm-usb.c | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) -- 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 net,stable] net: usb: Add HP hs2434 device to ZLP exception table
From 018343ce2e679d97283fb51da25c43aa876d087a Mon Sep 17 00:00:00 2001
From: Rob Gardner robma...@gmail.com
Date: Sun, 25 Aug 2013 16:02:23 -0600
Subject: [PATCH net,stable] net: usb: Add HP hs2434 device to ZLP exception
table
This patch adds another entry (HP hs2434 Mobile Broadband) to the list
of exceptional devices that require a zero length packet in order to
function properly. This list was added in commit 844e88f0. The hs2434
is manufactured by Sierra Wireless, who also produces the MC7710,
which the ZLP exception list was created for in the first place. So
hopefully it is just this one producer's devices that will need this
workaround.
Tested on a DM1-4310NR HP notebook, which does not function without this
change.
Signed-off-by: Rob Gardner robma...@gmail.com
---
drivers/net/usb/cdc_mbim.c |4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 8728198..25ba7ec 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -399,8 +399,12 @@ static const struct usb_device_id mbim_devs[] = {
/* Sierra Wireless MC7710 need ZLPs */
{ USB_DEVICE_AND_INTERFACE_INFO(0x1199, 0x68a2, USB_CLASS_COMM,
USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
.driver_info = (unsigned long)cdc_mbim_info_zlp,
},
+ /* HP hs2434 Mobile Broadband Module needs ZLPs */
+ { USB_DEVICE_AND_INTERFACE_INFO(0x3f0, 0x4b1d, USB_CLASS_COMM,
USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
+ .driver_info = (unsigned long)cdc_mbim_info_zlp,
+ },
{ USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM,
USB_CDC_PROTO_NONE),
.driver_info = (unsigned long)cdc_mbim_info,
},
{
--
1.7.9.5
--
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 net,stable] net: usb: Add HP hs2434 device to ZLP exception table
Rob Gardner robma...@gmail.com writes: From 018343ce2e679d97283fb51da25c43aa876d087a Mon Sep 17 00:00:00 2001 From: Rob Gardner robma...@gmail.com Date: Sun, 25 Aug 2013 16:02:23 -0600 Subject: [PATCH net,stable] net: usb: Add HP hs2434 device to ZLP exception table This patch adds another entry (HP hs2434 Mobile Broadband) to the list of exceptional devices that require a zero length packet in order to function properly. This list was added in commit 844e88f0. The hs2434 is manufactured by Sierra Wireless, who also produces the MC7710, which the ZLP exception list was created for in the first place. So hopefully it is just this one producer's devices that will need this workaround. Tested on a DM1-4310NR HP notebook, which does not function without this change. Signed-off-by: Rob Gardner robma...@gmail.com Acked-by: Bjørn Mork bj...@mork.no Thanks a lot for doing this! As I warned about in http://www.spinics.net/lists/netdev/msg223742.html I am now going to reconsider what do do about this issue. There are only two devices in the exception list so far, but I would like to note that this represents 100% of the Sierra Wireless MBIM devices we *know* are tested with this driver. Testing for this firmware bug and verifying the workaround on a new device is currently hard, requiring more skills than most end users can be expected to have. I therefore find it very likely that we will miss (or quite possible have missed already) a number of devices which should have been in this exception list. I believe we have to do something about this. Ideas are welcome. Bjørn -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] xHCI: Fixing xhci_readl definition and function call
This patch redefine function xhci_readl.xhci_readl function doesn't use
xhci_hcd argument.
Hence there is no need of keeping it in the function arguments.
Redefining this function breaks other functions which calls this function.
This phatch also correct those calls in xhci driver.
Signed-off-by: Kumar Gaurav kumargauravgup...@gmail.com
---
drivers/usb/host/xhci-dbg.c | 36 +++
drivers/usb/host/xhci-hub.c | 72 +++---
drivers/usb/host/xhci-mem.c | 20 -
drivers/usb/host/xhci-ring.c | 12 ++---
drivers/usb/host/xhci.c | 100 +-
drivers/usb/host/xhci.h |3 +-
6 files changed, 121 insertions(+), 122 deletions(-)
diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
index 5d5e58f..66dc1c0 100644
--- a/drivers/usb/host/xhci-dbg.c
+++ b/drivers/usb/host/xhci-dbg.c
@@ -32,7 +32,7 @@ void xhci_dbg_regs(struct xhci_hcd *xhci)
xhci_dbg(xhci, // xHCI capability registers at %p:\n,
xhci-cap_regs);
- temp = xhci_readl(xhci, xhci-cap_regs-hc_capbase);
+ temp = xhci_readl(xhci-cap_regs-hc_capbase);
xhci_dbg(xhci, // @%p = 0x%x (CAPLENGTH AND HCIVERSION)\n,
xhci-cap_regs-hc_capbase, temp);
xhci_dbg(xhci, // CAPLENGTH: 0x%x\n,
@@ -44,13 +44,13 @@ void xhci_dbg_regs(struct xhci_hcd *xhci)
xhci_dbg(xhci, // xHCI operational registers at %p:\n, xhci-op_regs);
- temp = xhci_readl(xhci, xhci-cap_regs-run_regs_off);
+ temp = xhci_readl(xhci-cap_regs-run_regs_off);
xhci_dbg(xhci, // @%p = 0x%x RTSOFF\n,
xhci-cap_regs-run_regs_off,
(unsigned int) temp RTSOFF_MASK);
xhci_dbg(xhci, // xHCI runtime registers at %p:\n, xhci-run_regs);
- temp = xhci_readl(xhci, xhci-cap_regs-db_off);
+ temp = xhci_readl(xhci-cap_regs-db_off);
xhci_dbg(xhci, // @%p = 0x%x DBOFF\n, xhci-cap_regs-db_off, temp);
xhci_dbg(xhci, // Doorbell array at %p:\n, xhci-dba);
}
@@ -61,7 +61,7 @@ static void xhci_print_cap_regs(struct xhci_hcd *xhci)
xhci_dbg(xhci, xHCI capability registers at %p:\n, xhci-cap_regs);
- temp = xhci_readl(xhci, xhci-cap_regs-hc_capbase);
+ temp = xhci_readl(xhci-cap_regs-hc_capbase);
xhci_dbg(xhci, CAPLENGTH AND HCIVERSION 0x%x:\n,
(unsigned int) temp);
xhci_dbg(xhci, CAPLENGTH: 0x%x\n,
@@ -69,7 +69,7 @@ static void xhci_print_cap_regs(struct xhci_hcd *xhci)
xhci_dbg(xhci, HCIVERSION: 0x%x\n,
(unsigned int) HC_VERSION(temp));
- temp = xhci_readl(xhci, xhci-cap_regs-hcs_params1);
+ temp = xhci_readl(xhci-cap_regs-hcs_params1);
xhci_dbg(xhci, HCSPARAMS 1: 0x%x\n,
(unsigned int) temp);
xhci_dbg(xhci, Max device slots: %u\n,
@@ -79,7 +79,7 @@ static void xhci_print_cap_regs(struct xhci_hcd *xhci)
xhci_dbg(xhci, Max ports: %u\n,
(unsigned int) HCS_MAX_PORTS(temp));
- temp = xhci_readl(xhci, xhci-cap_regs-hcs_params2);
+ temp = xhci_readl(xhci-cap_regs-hcs_params2);
xhci_dbg(xhci, HCSPARAMS 2: 0x%x\n,
(unsigned int) temp);
xhci_dbg(xhci, Isoc scheduling threshold: %u\n,
@@ -87,7 +87,7 @@ static void xhci_print_cap_regs(struct xhci_hcd *xhci)
xhci_dbg(xhci, Maximum allowed segments in event ring: %u\n,
(unsigned int) HCS_ERST_MAX(temp));
- temp = xhci_readl(xhci, xhci-cap_regs-hcs_params3);
+ temp = xhci_readl(xhci-cap_regs-hcs_params3);
xhci_dbg(xhci, HCSPARAMS 3 0x%x:\n,
(unsigned int) temp);
xhci_dbg(xhci, Worst case U1 device exit latency: %u\n,
@@ -95,14 +95,14 @@ static void xhci_print_cap_regs(struct xhci_hcd *xhci)
xhci_dbg(xhci, Worst case U2 device exit latency: %u\n,
(unsigned int) HCS_U2_LATENCY(temp));
- temp = xhci_readl(xhci, xhci-cap_regs-hcc_params);
+ temp = xhci_readl(xhci-cap_regs-hcc_params);
xhci_dbg(xhci, HCC PARAMS 0x%x:\n, (unsigned int) temp);
xhci_dbg(xhci, HC generates %s bit addresses\n,
HCC_64BIT_ADDR(temp) ? 64 : 32);
/* FIXME */
xhci_dbg(xhci, FIXME: more HCCPARAMS debugging\n);
- temp = xhci_readl(xhci, xhci-cap_regs-run_regs_off);
+ temp = xhci_readl(xhci-cap_regs-run_regs_off);
xhci_dbg(xhci, RTSOFF 0x%x:\n, temp RTSOFF_MASK);
}
@@ -110,7 +110,7 @@ static void xhci_print_command_reg(struct xhci_hcd *xhci)
{
u32 temp;
- temp = xhci_readl(xhci, xhci-op_regs-command);
+ temp = xhci_readl(xhci-op_regs-command);
xhci_dbg(xhci, USBCMD 0x%x:\n, temp);
xhci_dbg(xhci, HC is %s\n,
(temp CMD_RUN) ? running : being stopped);
@@ -128,7 +128,7 @@ static void
Re: Re: [PATCH] USB2NET : SR9700 : One chip USB 1.1 USB2NET SR9700Device Driver Support
On Mon, 2013-08-26 at 10:14 +0800, liujunliang_ljl wrote: do you need me to merge the patch and re-send it again. I do not. And which version of kernel will release this driver. No idea. That's up to David Miller or Greg KH to pick up the driver and maybe take the follow-on patch I sent. I just sent the patch because you seemed to have a bit of difficulty integrating the suggestions others were giving you. Thanks a lot and apologizing for making you trouble. Oh, no worries. It was a trifle. cheers, Joe -- 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: Re: Re: [PATCH] USB2NET : SR9700 : One chip USB 1.1 USB2NETSR9700Device Driver Support
Dear all : Thanks a lot. 2013-08-26 liujunliang_ljl 发件人： Joe Perches 发送时间： 2013-08-26 10:19:35 收件人： liujunliang_ljl 抄送： davem; horms; romieu; gregkh; netdev; linux-usb; linux-kernel; sunhecheng 主题： Re: Re: [PATCH] USB2NET : SR9700 : One chip USB 1.1 USB2NETSR9700Device Driver Support On Mon, 2013-08-26 at 10:14 +0800, liujunliang_ljl wrote: do you need me to merge the patch and re-send it again. I do not. And which version of kernel will release this driver. No idea. That's up to David Miller or Greg KH to pick up the driver and maybe take the follow-on patch I sent. I just sent the patch because you seemed to have a bit of difficulty integrating the suggestions others were giving you. Thanks a lot and apologizing for making you trouble. Oh, no worries. It was a trifle. cheers, Joe .
Re: Re: [PATCH] USB2NET : SR9700 : One chip USB 1.1 USB2NET SR9700Device Driver Support
DearJoe :
I'm sorry to ask you that, do you need me to merge the
patch and re-send it again.
And which version of kernel will release this driver.
Thanks a lot and apologizing for making you trouble.
Thanks again.
2013-08-26
liujunliang_ljl
发件人： Joe Perches
发送时间： 2013-08-25 02:15:19
收件人： liujunliang_ljl
抄送： davem; horms; romieu; gregkh; netdev; linux-usb; linux-kernel; sunhecheng
主题： Re: [PATCH] USB2NET : SR9700 : One chip USB 1.1 USB2NET SR9700Device Driver
Support
Some whitespace and neatening fixups.
Some conversions from 4 indent tabs to normal tabs
Signed-off-by: Joe Perches j...@perches.com
---
Just doing this instead of commenting about spacing
again.
drivers/net/usb/sr9700.c | 127 +--
1 file changed, 67 insertions(+), 60 deletions(-)
diff --git a/drivers/net/usb/sr9700.c b/drivers/net/usb/sr9700.c
index 27c86ec..4262b9d 100644
--- a/drivers/net/usb/sr9700.c
+++ b/drivers/net/usb/sr9700.c
@@ -29,7 +29,7 @@ static int sr_read(struct usbnet *dev, u8 reg, u16 length,
void *data)
int err;
err = usbnet_read_cmd(dev, SR_RD_REGS, SR_REQ_RD_REG,
- 0, reg, data, length);
+ 0, reg, data, length);
if ((err != length) (err = 0))
err = -EINVAL;
return err;
@@ -40,7 +40,7 @@ static int sr_write(struct usbnet *dev, u8 reg, u16 length,
void *data)
int err;
err = usbnet_write_cmd(dev, SR_WR_REGS, SR_REQ_WR_REG,
- 0, reg, data, length);
+0, reg, data, length);
if ((err = 0) (err length))
err = -EINVAL;
return err;
@@ -54,19 +54,19 @@ static int sr_read_reg(struct usbnet *dev, u8 reg, u8
*value)
static int sr_write_reg(struct usbnet *dev, u8 reg, u8 value)
{
return usbnet_write_cmd(dev, SR_WR_REGS, SR_REQ_WR_REG,
- value, reg, NULL, 0);
+ value, reg, NULL, 0);
}
static void sr_write_async(struct usbnet *dev, u8 reg, u16 length, void *data)
{
usbnet_write_cmd_async(dev, SR_WR_REGS, SR_REQ_WR_REG,
- 0, reg, data, length);
+0, reg, data, length);
}
static void sr_write_reg_async(struct usbnet *dev, u8 reg, u8 value)
{
usbnet_write_cmd_async(dev, SR_WR_REGS, SR_REQ_WR_REG,
- value, reg, NULL, 0);
+value, reg, NULL, 0);
}
static int wait_phy_eeprom_ready(struct usbnet *dev, int phy)
@@ -89,7 +89,7 @@ static int wait_phy_eeprom_ready(struct usbnet *dev, int phy)
if (i = SR_SHARE_TIMEOUT) {
netdev_err(dev-net, %s write timed out!\n,
- phy ? phy : eeprom);
+phy ? phy : eeprom);
ret = -EIO;
goto out;
}
@@ -98,7 +98,8 @@ out:
return ret;
}
-static int sr_share_read_word(struct usbnet *dev, int phy, u8 reg, __le16
*value)
+static int sr_share_read_word(struct usbnet *dev, int phy, u8 reg,
+ __le16 *value)
{
int ret;
@@ -115,14 +116,15 @@ static int sr_share_read_word(struct usbnet *dev, int
phy, u8 reg, __le16 *value
ret = sr_read(dev, EPDR, 2, value);
netdev_dbg(dev-net, read shared %d 0x%02x returned 0x%04x, %d\n,
- phy, reg, *value, ret);
+phy, reg, *value, ret);
out:
mutex_unlock(dev-phy_mutex);
return ret;
}
-static int sr_share_write_word(struct usbnet *dev, int phy, u8 reg, __le16
value)
+static int sr_share_write_word(struct usbnet *dev, int phy, u8 reg,
+__le16 value)
{
int ret;
@@ -156,7 +158,8 @@ static int sr9700_get_eeprom_len(struct net_device *dev)
return SR_EEPROM_LEN;
}
-static int sr9700_get_eeprom(struct net_device *net, struct ethtool_eeprom
*eeprom, u8 *data)
+static int sr9700_get_eeprom(struct net_device *net,
+ struct ethtool_eeprom *eeprom, u8 *data)
{
struct usbnet *dev = netdev_priv(net);
__le16 *ebuf = (__le16 *)data;
@@ -168,7 +171,8 @@ static int sr9700_get_eeprom(struct net_device *net, struct
ethtool_eeprom *eepr
return -EINVAL;
for (i = 0; i eeprom-len / 2; i++)
- ret = sr_read_eeprom_word(dev, eeprom-offset / 2 + i, ebuf[i]);
+ ret = sr_read_eeprom_word(dev, eeprom-offset / 2 + i,
+ ebuf[i]);
return ret;
}
@@ -199,12 +203,13 @@ static int sr_mdio_read(struct net_device *netdev, int
phy_id, int loc)
res = le16_to_cpu(res) ~BMSR_LSTATUS;
netdev_dbg(dev-net, sr_mdio_read() phy_id=0x%02x, loc=0x%02x,
returns=0x%04x\n,
- phy_id, loc, res);
+phy_id, loc, res);
return res;
}
-static void sr_mdio_write(struct net_device *netdev, int phy_id, int loc, int
val)
+static void sr_mdio_write(struct net_device *netdev, int phy_id, int loc,
+ int val)
{
struct usbnet *dev = netdev_priv(netdev);
__le16 res = cpu_to_le16(val);
@@ -215,7 +220,7 @@ static void sr_mdio_write(struct net_device *netdev, int
phy_id, int loc, int va
}
netdev_dbg(dev-net, sr_mdio_write() phy_id=0x%02x, loc=0x%02x,
val=0x%04x\n,
- phy_id, loc, val);
+phy_id, loc, val);
sr_share_write_word(dev, 1, loc, res);
}
@@ -242,15 +247,15 @@ static int sr9700_ioctl(struct net_device *net, struct
ifreq *rq, int cmd)
}
static const struct ethtool_ops
Re: [RFC 2/3] EHCI: convert the IRQ handler to a tasklet
On Sun, Aug 25, 2013 at 10:45 PM, Alan Stern st...@rowland.harvard.edu wrote: To begin with, the whole point of this RFC was to show that moving the entire IRQ handler (or even a large part of it) to a tasklet would have been at least as simple as moving the givebacks alone. Now that I realize the hrtimer and unlink pathways would have to be changed too, it's not quite so simple as it seemed at first. Still, I think it would be no worse than the work you did. It also is more typical of the way people expect the work to be divided between a top half and a bottom half -- usually the top half does little more than acknowledge the interrupt. I'd like to compare the two implementations when it is 'basically ready'. Since the proposal was meant only as a proof of principle, I'm not going to develope it any farther. But I will respond to the points you raised in your review. OK, I will ask Greg to put back the patch 'USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled' into his tree. On Sat, 24 Aug 2013, Ming Lei wrote: But the current interrupt from EHCI isn't be acked(USBSTS isn't cleared), so EHCI HW don't know the irq has been handled, then it is reasonable that the EHCI interrupt still interrupts CPU. EHCI spec(4.15) also states the point clearly: the host controller is not required to de-assert a currently active interrupt condition when software sets the interrupt enables (in the USBINR register, see Section 2.3.3) to a zero. The only reliable method software should use for acknowledging an interrupt is by transitioning the appropriate status bits in the USBSTS register (Section 2.3.2) from a one to a zero. Ooh, you're right! I had completely forgotten about this. It's a serious error in the RFC patch. Fixing it would be pretty easy, however. It is hard to say easy: - the USBSTS has to be cleared in top half, which means the status has to be stored somewhere(suppose it is ehci-irq_status) - ehci-irq_status need to be read in bottom half for handling irq, so one lock might be required for synchronizing the access on the variable - also, once the current irq is Acked by clearing USBSTS, then later interrupts can come, so the irq status should have been saved into one queue instead of single variable(see below why disabling ehci irq isn't good) So when the above implementation is required in each HCDs change, I am wondering if it is 'pretty easy'. Not handling interrupts right away is the price we pay for using a bottom half. Your tasklet implementation isn't very different. Although the interrupt handler may realize quickly that a transfer has finished, there will still be a delay before the URB's completion handler can run. And the length of that delay will depend on the tasklet schedule delay. In my tasklet implementation, the next EHCI interrupt can be handled after the current hard interrupt is handled, but in this patch the EHCI hard irq can't be handled until the tasklet is handled, right? Right. But in the end, it doesn't matter. Remember, more than 99% of the work an HCD does is handling URBs. (HCDs do a couple of other things, such as detecting connections and disconnection or handling wakeups, but they are very small compared to the number of URBs it handles.) Consider also that an URB isn't done until it is given back to the driver that submitted it. Therefore, when measuring performance or latency of an HCD, what matters is how long the driver has to wait from URB submission to URB completion. If any part of that processing takes place in a tasklet, the tasklet delay will add in to the total time. It doesn't matter whether the tasklet handles only the giveback or some of the HCD's internal work as well. I am not sure if the idea of disabling EHCI irq inside ehci_irq() is good: one completed transfer may not be observed inside current tasklet handler since hardware interrupt is disabled, so the transfer can't be handled until next tasklet scheduled, then extra delay for these URBs is introduced. Thirdly, fatal error should have been handled immediately inside hard interrupt handler. Why? What difference does it make? At least, EHCI HW generates the fatal irq ASAP, This interrupt is not delayed to the next interrupt threshold.(4.15.2.4), so it is reasonable for software to handle it asap. That doesn't seem like a good reason. (Probably it just reflects the fact that once a fatal error has occurred, the controller hardware can't reliably determine when the next interrupt threshold will occur.) But of course, it would be easy to put that part of the code into the top half of the handler. More things done in top half, the change will become more complicated since more synchronizations need to consider. Finally, tasklet schedule should have been optimized here if the tasklet is running on another CPU,
