Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()
Hi
On 3/8/2019 12:09 AM, Mark Brown wrote:
On Thu, Mar 07, 2019 at 05:26:53PM +0200, Jarkko Nikula wrote:
On 3/7/19 9:24 AM, xiao jin wrote:
The patch is to do cs again if spi-pxa2xx restar the SSP during
pxa2xx_spi_transfer_one()
Hmm.. please correct me if I'm wrong but pxa2xx_spi_unprepare_transfer() is
called always when there is no more messages pending and the spi core should
have deasserted the CS already?
Yes.
I try to describe the situation in more detail.
cpu0 cpu1
=> spi_transfer_one_message
=> __spi_pump_messages
=> __spi_sync
=> spi_sync
=> spi_sync_transfer.constprop.2
=> spi_write
=> fm1388_spi_burst_write
=> fm1388_fw_loaded
=> request_firmware_work_func
=> process_one_work
=> pxa2xx_spi_unprepare_transfer
=> spi_pump_messages
=> kthread_worker_fn
Yes, the spi core has de-asserted the CS before the
pxa2xx_spi_unprepare_transfer(). The problem on my side is that the new
transfer will restart the SSP in pxa2xx_spi_transfer_one(). The spi core
has asserted the CS again before restart the SSP. In my test the CS
assert before the restart SSP can't ensure the spi transfer working
correctly.
@@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller
*master,
if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
|| (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
!= (cr1 & change_mask)) {
+ /* It needs to deassert the chip selection
+* firstly before restart the SPP */
+ need_cs_change = true;
+ cs_deassert(spi);
I think code comes here at the beginning of each transfer so will be hit
multiple times before pxa2xx_spi_unprepare_transfer() if SPI message
consists of multiple transfers.
This makes me wondering if the device driver setting up the "struct
spi_transfer" is maybe missing the cs_change flag set for transfers before
last one in case HW needs CS toggling between transfers? For instance what
following drivers are doing with the cs_change flag:
drivers/char/tpm/tpm_tis_spi.c: tpm_tis_spi_transfer()
drivers/input/touchscreen/ad7877.c: ad7877_read(), ad7877_read_adc()
Right, this really feels like it's fixing the wrong thing.
I check the cs_change flag in the spi_transfer_one_message(). The
cs_change flag is used in two cases. If the transfer is the last one it
is used to keep the CS assert. If the transfer is not the last one it's
used to generate the 10us pulse and then de-assert the CS. In my case
the transfer is the last one and it can work correctly if I re-assert
the CS after restart the SSP in the pxa2xx_spi_transfer_one() which is
called from spi_transfer_one_message(). From my experiments both
cs_change flag cases in the spi_transfer_one_message() can't help the
problem. I am not sure if I fully understand your point.
[PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()
The spi-pxa2xx can't read and write data correctly on our board.
The pxa_ssp_type is LPSS_BXT_SSP in our case.
With more debug we find that it's related to restart the SPP
during pxa2xx_spi_transfer_one().
In the normal case the spi_transfer_one_message() calls spi-pxa2xx
cs_assert before transferring one message. After completing the
transfer it calls spi-pxa2xx cs_deassert. The spi-pxa2xx works
well.
But in some other case pxa2xx_spi_unprepare_transfer() is called
that clears SSCR0_SSE bit before the next transfer. In the next
transfer the spi-pxa2xx driver will restart the SSP as the SSE
bit is cleared. The cs_assert before the SSP restart can't ensure
spi-pxa2xx work well.
The patch is to do cs again if spi-pxa2xx restar the SSP during
pxa2xx_spi_transfer_one()
Signed-off-by: xiao jin
Signed-off-by: he, bo
---
drivers/spi/spi-pxa2xx.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 14f4ea59caff..1a2ea46858d9 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -928,6 +928,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller
*master,
u32 cr1;
int err;
int dma_mapped;
+ bool need_cs_change = false;
/* Check if we can DMA this transfer */
if (transfer->len > MAX_DMA_LEN && chip->enable_dma) {
@@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller
*master,
if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
|| (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
!= (cr1 & change_mask)) {
+ /* It needs to deassert the chip selection
+* firstly before restart the SPP */
+ need_cs_change = true;
+ cs_deassert(spi);
+
/* stop the SSP, and update the other bits */
pxa2xx_spi_write(drv_data, SSCR0, cr0 & ~SSCR0_SSE);
if (!pxa25x_ssp_comp(drv_data))
@@ -1070,6 +1076,8 @@ static int pxa2xx_spi_transfer_one(struct spi_controller
*master,
pxa2xx_spi_write(drv_data, SSTO, chip->timeout);
}
+ if (need_cs_change)
+ cs_assert(spi);
/*
* Release the data by enabling service requests and interrupts,
* without changing any mode bits
--
2.21.0
Re: [PATCH] spi-pxa2xx.c: modify the chip selection timing when spi transfer
Hi Jarkko,
On 3/6/2019 3:52 PM, Jarkko Nikula wrote:
Hi
On 3/6/19 5:05 AM, xiao jin wrote:
From: "he, bo"
We find spi can't work on board. More debug shows it's related
to the following patch that changed the chip selection assert and
deassert timing.
^^ timing caught my attention. More below.
@@ -610,6 +596,7 @@ static void int_transfer_complete(struct
driver_data *drv_data)
if (!pxa25x_ssp_comp(drv_data))
pxa2xx_spi_write(drv_data, SSTO, 0);
+ cs_deassert(drv_data);
spi_finalize_current_transfer(drv_data->master);
This
@@ -1070,6 +1057,7 @@ static int pxa2xx_spi_transfer_one(struct
spi_controller *master,
pxa2xx_spi_write(drv_data, SSTO, chip->timeout);
}
+ cs_assert(drv_data);
and this is not correct with core message loop. It will cause the chip
select is toggled with each transfer in PIO mode. If there is no
cs_change flag set then there shouldn't be CS toggling between the
transfers if SPI message consists of multiple transfers.
More over this patch also will regress with DMA mode since there won't
be CS deassert at all.
Timing reminded me I've seen two cases where there was a timing
related glitch in CS output:
d0283eb2dbc1 ("spi: pxa2xx: Add output control for multiple Intel LPSS
chip selects")
7a8d44bc89e5 ("spi: pxa2xx: Fix too early chipselect deassert")
Do you have a possibility to measure with an oscilloscope what goes
wrong with the CS after d5898e19c0d7 ("spi: pxa2xx: Use core message
processing loop")?
Can you share your setup if I can reproduce it here? E.g. SPI clock
frequency, single or multiple CS, frequency of occurrence, etc
In our setup the pxa_ssp_type is LPSS_BXT_SSP. With more debug we find
that the problem is caused in the case of "restart the SSP" when
pxa2xx_spi_transfer_one.
We will send another RFC patch.
Thanks.
[PATCH] spi-pxa2xx.c: modify the chip selection timing when spi transfer
From: "he, bo"
We find spi can't work on board. More debug shows it's related
to the following patch that changed the chip selection assert and
deassert timing.
commit d5898e19c0d74cd41b9f5c8c8ea87e559c3fe0c1
spi: pxa2xx: Use core message processing loop
Convert the pump_transfers() transfer tasklet to transfer_one()
hook the SPI core calls to process single transfer instead of
handling message processing and chip select handling in the driver.
This not only simplifies the driver but also brings transfer
statistics from the core.
We modify the chip selection timeing action back to before the patch.
The spi on the board works.
Signed-off-by: xiao jin
Signed-off-by: he, bo
---
drivers/spi/spi-pxa2xx.c | 41 ++--
1 file changed, 14 insertions(+), 27 deletions(-)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index ec4b65100..0affa09da 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -340,11 +340,9 @@ static void lpss_ssp_setup(struct driver_data *drv_data)
}
}
-static void lpss_ssp_select_cs(struct spi_device *spi,
+static void lpss_ssp_select_cs(struct driver_data *drv_data,
const struct lpss_config *config)
{
- struct driver_data *drv_data =
- spi_controller_get_devdata(spi->controller);
u32 value, cs;
if (!config->cs_sel_mask)
@@ -352,7 +350,7 @@ static void lpss_ssp_select_cs(struct spi_device *spi,
value = __lpss_ssp_read_priv(drv_data, config->reg_cs_ctrl);
- cs = spi->chip_select;
+ cs = drv_data->master->cur_msg->spi->chip_select;
cs <<= config->cs_sel_shift;
if (cs != (value & config->cs_sel_mask)) {
/*
@@ -371,17 +369,15 @@ static void lpss_ssp_select_cs(struct spi_device *spi,
}
}
-static void lpss_ssp_cs_control(struct spi_device *spi, bool enable)
+static void lpss_ssp_cs_control(struct driver_data *drv_data, bool enable)
{
- struct driver_data *drv_data =
- spi_controller_get_devdata(spi->controller);
const struct lpss_config *config;
u32 value;
config = lpss_get_config(drv_data);
if (enable)
- lpss_ssp_select_cs(spi, config);
+ lpss_ssp_select_cs(drv_data, config);
value = __lpss_ssp_read_priv(drv_data, config->reg_cs_ctrl);
if (enable)
@@ -391,11 +387,10 @@ static void lpss_ssp_cs_control(struct spi_device *spi,
bool enable)
__lpss_ssp_write_priv(drv_data, config->reg_cs_ctrl, value);
}
-static void cs_assert(struct spi_device *spi)
+static void cs_assert(struct driver_data *drv_data)
{
- struct chip_data *chip = spi_get_ctldata(spi);
- struct driver_data *drv_data =
- spi_controller_get_devdata(spi->controller);
+ struct chip_data *chip =
+ spi_get_ctldata(drv_data->master->cur_msg->spi);
if (drv_data->ssp_type == CE4100_SSP) {
pxa2xx_spi_write(drv_data, SSSR, chip->frm);
@@ -413,14 +408,13 @@ static void cs_assert(struct spi_device *spi)
}
if (is_lpss_ssp(drv_data))
- lpss_ssp_cs_control(spi, true);
+ lpss_ssp_cs_control(drv_data, true);
}
-static void cs_deassert(struct spi_device *spi)
+static void cs_deassert(struct driver_data *drv_data)
{
- struct chip_data *chip = spi_get_ctldata(spi);
- struct driver_data *drv_data =
- spi_controller_get_devdata(spi->controller);
+ struct chip_data *chip =
+ spi_get_ctldata(drv_data->master->cur_msg->spi);
unsigned long timeout;
if (drv_data->ssp_type == CE4100_SSP)
@@ -443,15 +437,7 @@ static void cs_deassert(struct spi_device *spi)
}
if (is_lpss_ssp(drv_data))
- lpss_ssp_cs_control(spi, false);
-}
-
-static void pxa2xx_spi_set_cs(struct spi_device *spi, bool level)
-{
- if (level)
- cs_deassert(spi);
- else
- cs_assert(spi);
+ lpss_ssp_cs_control(drv_data, false);
}
int pxa2xx_spi_flush(struct driver_data *drv_data)
@@ -610,6 +596,7 @@ static void int_transfer_complete(struct driver_data
*drv_data)
if (!pxa25x_ssp_comp(drv_data))
pxa2xx_spi_write(drv_data, SSTO, 0);
+ cs_deassert(drv_data);
spi_finalize_current_transfer(drv_data->master);
}
@@ -1070,6 +1057,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller
*master,
pxa2xx_spi_write(drv_data, SSTO, chip->timeout);
}
+ cs_assert(drv_data);
/*
* Release the data by enabling service requests and interrupts,
* without changing any mode bits
@@ -1563,7 +1551,6 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
maste
Re: [PATCH] smpboot.c: move setup_vector_irq after set_cpu_online
Hi tglx, On 7/2/2015 8:50 PM, Thomas Gleixner wrote: The whole vector stuff is racy versus cpu hotplug and Jins patch merily addresses a small part of it and by doing that it breaks stuff as well. With that patch we move the vector setup after marking the cpu online, which is wrong because the vector array on that cpu is not up to date until we call __setup_vector_irq(). Proper patch below. We still have an issue in the cpu_disable() patch, but I haven't yet wrapped my head around it completely. Thanks, tglx Your patch is better. Thanks. Jin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] smpboot.c: move setup_vector_irq after set_cpu_online
Hi Joerg, On 7/2/2015 2:52 PM, Joerg Roedel wrote: Hi Jin, On Thu, Jul 02, 2015 at 12:24:34PM +0800, xiao jin wrote: [ 106.107851] BUG: unable to handle kernel NULL pointer dereference at 0040 [ 106.116702] IP: [ 106.118490] [] check_irq_vectors_for_cpu_disable+0x76/0x180 This might be the same issue I fixed with: d97eb8966c91 x86/irq: Check for valid irq descriptor in check_irq_vectors_for_cpu_disable() Can you try whether applying this patch on your kernel fixes your issue? Yes, I agree d97eb8966c91 fix the IPANIC. I just notice a description from http://lkml.kernel.org/r/20150204132754.ga10...@suse.de. I wish to share the debug result and root cause from my side. Thanks, Joerg Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] smpboot.c: move setup_vector_irq after set_cpu_online
Hi Joerg, On 7/2/2015 2:52 PM, Joerg Roedel wrote: Hi Jin, On Thu, Jul 02, 2015 at 12:24:34PM +0800, xiao jin wrote: [ 106.107851] BUG: unable to handle kernel NULL pointer dereference at 0040 [ 106.116702] IP: [ 106.118490] [810044f6] check_irq_vectors_for_cpu_disable+0x76/0x180 This might be the same issue I fixed with: d97eb8966c91 x86/irq: Check for valid irq descriptor in check_irq_vectors_for_cpu_disable() Can you try whether applying this patch on your kernel fixes your issue? Yes, I agree d97eb8966c91 fix the IPANIC. I just notice a description from http://lkml.kernel.org/r/20150204132754.ga10...@suse.de. I wish to share the debug result and root cause from my side. Thanks, Joerg Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] smpboot.c: move setup_vector_irq after set_cpu_online
Hi tglx, On 7/2/2015 8:50 PM, Thomas Gleixner wrote: The whole vector stuff is racy versus cpu hotplug and Jins patch merily addresses a small part of it and by doing that it breaks stuff as well. With that patch we move the vector setup after marking the cpu online, which is wrong because the vector array on that cpu is not up to date until we call __setup_vector_irq(). Proper patch below. We still have an issue in the cpu_disable() patch, but I haven't yet wrapped my head around it completely. Thanks, tglx Your patch is better. Thanks. Jin -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] smpboot.c: move setup_vector_irq after set_cpu_online
r_irq after set_cpu_online. Signed-off-by: xiao jin --- arch/x86/kernel/smpboot.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 50e547e..f7d5d79 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -172,11 +172,6 @@ static void smp_callin(void) apic_ap_setup(); /* -* Need to setup vector mappings before we enable interrupts. -*/ - setup_vector_irq(smp_processor_id()); - - /* * Save our processor parameters. Note: this information * is needed for clock calibration. */ @@ -257,6 +252,11 @@ static void notrace start_secondary(void *unused) cpu_set_state_online(smp_processor_id()); x86_platform.nmi_init(); + /* +* Need to setup vector mappings before we enable interrupts. +*/ + setup_vector_irq(smp_processor_id()); + /* enable local interrupts */ local_irq_enable(); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] smpboot.c: move setup_vector_irq after set_cpu_online
= native_teardown_msi_irq = destroy_irq = __clear_irq_vector = set_cpu_online The cpu still is not online when clear irq vector, it makes the irq number remain in irq vector after free_msi_irqs. Next native_cpu_disable() will hit NULL pointer when check irq vector. The patch move setup_vector_irq after set_cpu_online. Signed-off-by: xiao jin jin.x...@intel.com --- arch/x86/kernel/smpboot.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 50e547e..f7d5d79 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -172,11 +172,6 @@ static void smp_callin(void) apic_ap_setup(); /* -* Need to setup vector mappings before we enable interrupts. -*/ - setup_vector_irq(smp_processor_id()); - - /* * Save our processor parameters. Note: this information * is needed for clock calibration. */ @@ -257,6 +252,11 @@ static void notrace start_secondary(void *unused) cpu_set_state_online(smp_processor_id()); x86_platform.nmi_init(); + /* +* Need to setup vector mappings before we enable interrupts. +*/ + setup_vector_irq(smp_processor_id()); + /* enable local interrupts */ local_irq_enable(); -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: ehci-hub: wait for RESUME finished when hub try to clear SUSPEND
On 05/04/2014 08:26 AM, Peter Chen wrote: On Sat, May 03, 2014 at 11:52:25AM +0800, xiao jin wrote: We use usb ehci to connect with modem and run stress test on ehci remote wake. Sometimes usb disconnect. We add more debug ftrace (Kernel version: 3.10) and list the key log to show how problem happened. -0 [000] d.h2 26879.385095: ehci_irq: irq status 1008c PPCE FLR PCD -0 [000] d.h2 26879.385099: ehci_irq: rh_state[2] hcd->state[132] pstatus[0][238014c5] suspended_ports[1] reset_done[0] <...>-12873 [000] d..1 26879.393536: ehci_hub_control: GetStatus port:1 status 238014c5 17 ERR POWER sig=k SUSPEND RESUME PE CONNECT <...>-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1] wValue[2] <...>-12873 [000] d..1 26879.393553: ehci_hub_control: [ehci_hub_control]line[891] port[0] hostpc_reg [44000202]->[44000202] -0 [001] ..s. 26879.403122: ehci_hub_status_data: wgq[ehci_hub_status_data] ignore_oc[0] resuming_ports[1] <...>-12873 [000] d..1 26879.413379: ehci_hub_control: [ehci_hub_control]line[907] port[0] write portsc_reg[238014c5] reset_done[2105769] <...>-12873 [000] d..1 26879.453173: ehci_hub_control: GetStatus port:1 status 23801885 17 ERR POWER sig=j SUSPEND PE CONNECT <...>-12873 [000] 26879.473158: check_port_resume_type: port 1 status .0507 after resume, -19 <...>-12873 [000] 26879.473160: usb_port_resume: status = -19 after check_port_resume_type <...>-12873 [000] 26879.473161: usb_port_resume: can't resume, status -19 <...>-12873 [000] 26879.473162: hub_port_logical_disconnect: logical disconnect on port 1 There is a in-band remote wakeup and controller run in k-state. Then kernel driver(ClearPortFeature/USB_PORT_FEAT_SUSPEND) write RESUME|LS(k-state) bit into controller. It makes controller status weird. It's defined in EHCI Are you sure you are at this path? If there is a remote wakeup, the sending resume signal from host controller will be skipped (USB_PORT_FEAT_SUSPEND), see usb_port_resume at drivers/usb/core/hub.c. Yes, I abstract more debug log to explain. <...>-12873 [000] 26879.393496: usb_port_resume: wgq[usb_port_resume] <...>-12873 [000] 26879.393544: usb_port_resume: status = 0 after hub_port_status <...>-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1] wValue[2] <...>-12873 [000] d..1 26879.413379: ehci_hub_control: [ehci_hub_control]line[907] port[0] write portsc_reg[238014c5] reset_done[2105769] <...>-12873 [000] 26879.413401: usb_port_resume: status = 0 after clear_port_feature controller spec(Revision 1.0), "If it has enabled remote wake-up, a K-state on the bus will turn the transceiver clock and generate an interrupt. The software will then have to wait 20 ms for the resume to complete and the port to go back to an active state." In this case Kernel should wait for the wakeup finished, then judge what should do next step. Do you use a chipidea core? Try to do not clear run/stop to see if this problem is fixed or not. I have explained more about the problem in another mail. Please have a look if there still need more info. Jin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: ehci-hub: wait for RESUME finished when hub try to clear SUSPEND
On 05/03/2014 11:20 PM, Alan Stern wrote: On Sat, 3 May 2014, xiao jin wrote: We use usb ehci to connect with modem and run stress test on ehci remote wake. Sometimes usb disconnect. We add more debug ftrace (Kernel version: 3.10) and list the key log to show how problem happened. -0 [000] d.h2 26879.385095: ehci_irq: irq status 1008c PPCE FLR PCD -0 [000] d.h2 26879.385099: ehci_irq: rh_state[2] hcd->state[132] pstatus[0][238014c5] suspended_ports[1] reset_done[0] => kernel receive a remote wakeup irq from controller <...>-12873 [000] d..1 26879.393536: ehci_hub_control: GetStatus port:1 status 238014c5 17 ERR POWER sig=k SUSPEND RESUME PE CONNECT => PORTSC = 238014c5 (line status = K-state, suspend = 1, force port resume = 1, J-to-K transition is detected) <...>-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1] wValue[2] <...>-12873 [000] d..1 26879.393553: ehci_hub_control: [ehci_hub_control]line[891] port[0] hostpc_reg [44000202]->[44000202] -0 [001] ..s. 26879.403122: ehci_hub_status_data: wgq[ehci_hub_status_data] ignore_oc[0] resuming_ports[1] <...>-12873 [000] d..1 26879.413379: ehci_hub_control: [ehci_hub_control]line[907] port[0] write portsc_reg[238014c5] reset_done[2105769] => kernel write 238014c5 to PORTSC <...>-12873 [000] d..1 26879.453173: ehci_hub_control: GetStatus port:1 status 23801885 17 ERR POWER sig=j SUSPEND PE CONNECT => PORTSC = 23801885 (line status = J-state, suspend = 1), is the status weird? <...>-12873 [000] 26879.473158: check_port_resume_type: port 1 status .0507 after resume, -19 <...>-12873 [000] 26879.473160: usb_port_resume: status = -19 after check_port_resume_type <...>-12873 [000] 26879.473161: usb_port_resume: can't resume, status -19 <...>-12873 [000] 26879.473162: hub_port_logical_disconnect: logical disconnect on port 1 This is a little difficult to understand... We add some debug log manually, please let me explain a little more. See above "=>". There is a in-band remote wakeup and controller run in k-state. Then kernel What do you mean by "in-band"? We use EHCI as host and have two kinds of mechanism to remote wakeup event, "in-band" is ehci controller self wakeup, sorry to make you misunderstanding. driver(ClearPortFeature/USB_PORT_FEAT_SUSPEND) write RESUME|LS(k-state) bit into controller. Why did it do that? Did the kernel try to resume the port at the same time as the device issued a remote wakeup request? In other words, was there a race between resume and remote wakeup? Yes, I mean a race between kernel driver resume and controller remote wakeup. It makes controller status weird. Why? Your log shows that the RESUME bit was already turned on, so writing a 1 to it shouldn't make any difference. (And the LS(k-state) bit is RO, so writing a 1 to it shouldn't matter.) Here the problem is, after remote wakeup, the controller still is in SUSPEND and kernel report disconnect finally. Could kernel write "SUSPEND" or "Force Port Resume" bit be related to the problem we meet with? It's defined in EHCI controller spec(Revision 1.0), "If it has enabled remote wake-up, a K-state on the bus will turn the transceiver clock and generate an interrupt. The software will then have to wait 20 ms for the resume to complete and the port to go back to an active state." Where in the spec did you find that quote? It's not present in my copy of the EHCI Rev 1.0 spec. I am sorry to make a mistake, I quote it from controller reference manual. I can find the similar definition in EHCI Rev 1.0, 4.3.1 Port Suspend/Resume. "When Force Port Resume bit is a one, the host controller sends resume signaling down the port. System software times the duration of the resume (nominally 20 milliseconds) then sets the Force Port Resume bit to a zero." In this case Kernel should wait for the wakeup finished, then judge what should do next step. We have some thought and give a patch. This patch is to wait for controller RESUME finished when hub try to clear port SUSPEND feature. This is definitely wrong. For one thing, you mustn't have a 20-ms delay with interrupts disabled. For another, the spec states (Table 2-16, the entry for bits 11:10) that the Line Status value is valid only when the port enable bit is 0, so you shouldn't rely on it. Lastly, there really is no need to do anything, because the remote wakeup will finish all by itself. I agree disable irq for maximum 20-ms is not good, but I can find another case when ehci_hub_control() deal with GetPortStatus. I have no idea how controller run, from both EHCI spec and reference manual, I can only deduce that it's better kernel driver don't touch PORTSC until resume finished. Otherwise how to explain the problem we meet with? (After remo
Re: [PATCH] USB: ehci-hub: wait for RESUME finished when hub try to clear SUSPEND
On 05/03/2014 11:20 PM, Alan Stern wrote: On Sat, 3 May 2014, xiao jin wrote: We use usb ehci to connect with modem and run stress test on ehci remote wake. Sometimes usb disconnect. We add more debug ftrace (Kernel version: 3.10) and list the key log to show how problem happened. idle-0 [000] d.h2 26879.385095: ehci_irq: irq status 1008c PPCE FLR PCD idle-0 [000] d.h2 26879.385099: ehci_irq: rh_state[2] hcd-state[132] pstatus[0][238014c5] suspended_ports[1] reset_done[0] = kernel receive a remote wakeup irq from controller ...-12873 [000] d..1 26879.393536: ehci_hub_control: GetStatus port:1 status 238014c5 17 ERR POWER sig=k SUSPEND RESUME PE CONNECT = PORTSC = 238014c5 (line status = K-state, suspend = 1, force port resume = 1, J-to-K transition is detected) ...-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1] wValue[2] ...-12873 [000] d..1 26879.393553: ehci_hub_control: [ehci_hub_control]line[891] port[0] hostpc_reg [44000202]-[44000202] idle-0 [001] ..s. 26879.403122: ehci_hub_status_data: wgq[ehci_hub_status_data] ignore_oc[0] resuming_ports[1] ...-12873 [000] d..1 26879.413379: ehci_hub_control: [ehci_hub_control]line[907] port[0] write portsc_reg[238014c5] reset_done[2105769] = kernel write 238014c5 to PORTSC ...-12873 [000] d..1 26879.453173: ehci_hub_control: GetStatus port:1 status 23801885 17 ERR POWER sig=j SUSPEND PE CONNECT = PORTSC = 23801885 (line status = J-state, suspend = 1), is the status weird? ...-12873 [000] 26879.473158: check_port_resume_type: port 1 status .0507 after resume, -19 ...-12873 [000] 26879.473160: usb_port_resume: status = -19 after check_port_resume_type ...-12873 [000] 26879.473161: usb_port_resume: can't resume, status -19 ...-12873 [000] 26879.473162: hub_port_logical_disconnect: logical disconnect on port 1 This is a little difficult to understand... We add some debug log manually, please let me explain a little more. See above =. There is a in-band remote wakeup and controller run in k-state. Then kernel What do you mean by in-band? We use EHCI as host and have two kinds of mechanism to remote wakeup event, in-band is ehci controller self wakeup, sorry to make you misunderstanding. driver(ClearPortFeature/USB_PORT_FEAT_SUSPEND) write RESUME|LS(k-state) bit into controller. Why did it do that? Did the kernel try to resume the port at the same time as the device issued a remote wakeup request? In other words, was there a race between resume and remote wakeup? Yes, I mean a race between kernel driver resume and controller remote wakeup. It makes controller status weird. Why? Your log shows that the RESUME bit was already turned on, so writing a 1 to it shouldn't make any difference. (And the LS(k-state) bit is RO, so writing a 1 to it shouldn't matter.) Here the problem is, after remote wakeup, the controller still is in SUSPEND and kernel report disconnect finally. Could kernel write SUSPEND or Force Port Resume bit be related to the problem we meet with? It's defined in EHCI controller spec(Revision 1.0), If it has enabled remote wake-up, a K-state on the bus will turn the transceiver clock and generate an interrupt. The software will then have to wait 20 ms for the resume to complete and the port to go back to an active state. Where in the spec did you find that quote? It's not present in my copy of the EHCI Rev 1.0 spec. I am sorry to make a mistake, I quote it from controller reference manual. I can find the similar definition in EHCI Rev 1.0, 4.3.1 Port Suspend/Resume. When Force Port Resume bit is a one, the host controller sends resume signaling down the port. System software times the duration of the resume (nominally 20 milliseconds) then sets the Force Port Resume bit to a zero. In this case Kernel should wait for the wakeup finished, then judge what should do next step. We have some thought and give a patch. This patch is to wait for controller RESUME finished when hub try to clear port SUSPEND feature. This is definitely wrong. For one thing, you mustn't have a 20-ms delay with interrupts disabled. For another, the spec states (Table 2-16, the entry for bits 11:10) that the Line Status value is valid only when the port enable bit is 0, so you shouldn't rely on it. Lastly, there really is no need to do anything, because the remote wakeup will finish all by itself. I agree disable irq for maximum 20-ms is not good, but I can find another case when ehci_hub_control() deal with GetPortStatus. I have no idea how controller run, from both EHCI spec and reference manual, I can only deduce that it's better kernel driver don't touch PORTSC until resume finished. Otherwise how to explain the problem we meet with? (After remote wakeup, the controller still is in SUSPEND and kernel report disconnect finally.) When we try the original change in the mail, we never see the same problem until
Re: [PATCH] USB: ehci-hub: wait for RESUME finished when hub try to clear SUSPEND
On 05/04/2014 08:26 AM, Peter Chen wrote: On Sat, May 03, 2014 at 11:52:25AM +0800, xiao jin wrote: We use usb ehci to connect with modem and run stress test on ehci remote wake. Sometimes usb disconnect. We add more debug ftrace (Kernel version: 3.10) and list the key log to show how problem happened. idle-0 [000] d.h2 26879.385095: ehci_irq: irq status 1008c PPCE FLR PCD idle-0 [000] d.h2 26879.385099: ehci_irq: rh_state[2] hcd-state[132] pstatus[0][238014c5] suspended_ports[1] reset_done[0] ...-12873 [000] d..1 26879.393536: ehci_hub_control: GetStatus port:1 status 238014c5 17 ERR POWER sig=k SUSPEND RESUME PE CONNECT ...-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1] wValue[2] ...-12873 [000] d..1 26879.393553: ehci_hub_control: [ehci_hub_control]line[891] port[0] hostpc_reg [44000202]-[44000202] idle-0 [001] ..s. 26879.403122: ehci_hub_status_data: wgq[ehci_hub_status_data] ignore_oc[0] resuming_ports[1] ...-12873 [000] d..1 26879.413379: ehci_hub_control: [ehci_hub_control]line[907] port[0] write portsc_reg[238014c5] reset_done[2105769] ...-12873 [000] d..1 26879.453173: ehci_hub_control: GetStatus port:1 status 23801885 17 ERR POWER sig=j SUSPEND PE CONNECT ...-12873 [000] 26879.473158: check_port_resume_type: port 1 status .0507 after resume, -19 ...-12873 [000] 26879.473160: usb_port_resume: status = -19 after check_port_resume_type ...-12873 [000] 26879.473161: usb_port_resume: can't resume, status -19 ...-12873 [000] 26879.473162: hub_port_logical_disconnect: logical disconnect on port 1 There is a in-band remote wakeup and controller run in k-state. Then kernel driver(ClearPortFeature/USB_PORT_FEAT_SUSPEND) write RESUME|LS(k-state) bit into controller. It makes controller status weird. It's defined in EHCI Are you sure you are at this path? If there is a remote wakeup, the sending resume signal from host controller will be skipped (USB_PORT_FEAT_SUSPEND), see usb_port_resume at drivers/usb/core/hub.c. Yes, I abstract more debug log to explain. ...-12873 [000] 26879.393496: usb_port_resume: wgq[usb_port_resume] ...-12873 [000] 26879.393544: usb_port_resume: status = 0 after hub_port_status ...-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1] wValue[2] ...-12873 [000] d..1 26879.413379: ehci_hub_control: [ehci_hub_control]line[907] port[0] write portsc_reg[238014c5] reset_done[2105769] ...-12873 [000] 26879.413401: usb_port_resume: status = 0 after clear_port_feature controller spec(Revision 1.0), If it has enabled remote wake-up, a K-state on the bus will turn the transceiver clock and generate an interrupt. The software will then have to wait 20 ms for the resume to complete and the port to go back to an active state. In this case Kernel should wait for the wakeup finished, then judge what should do next step. Do you use a chipidea core? Try to do not clear run/stop to see if this problem is fixed or not. I have explained more about the problem in another mail. Please have a look if there still need more info. Jin -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] USB: ehci-hub: wait for RESUME finished when hub try to clear SUSPEND
We use usb ehci to connect with modem and run stress test on ehci
remote wake. Sometimes usb disconnect. We add more debug ftrace
(Kernel version: 3.10) and list the key log to show how problem
happened.
-0 [000] d.h2 26879.385095: ehci_irq: irq status 1008c PPCE FLR PCD
-0 [000] d.h2 26879.385099: ehci_irq: rh_state[2] hcd->state[132]
pstatus[0][238014c5] suspended_ports[1] reset_done[0]
<...>-12873 [000] d..1 26879.393536: ehci_hub_control: GetStatus port:1 status
238014c5 17 ERR POWER sig=k SUSPEND RESUME PE CONNECT
<...>-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1]
wValue[2]
<...>-12873 [000] d..1 26879.393553: ehci_hub_control:
[ehci_hub_control]line[891] port[0] hostpc_reg [44000202]->[44000202]
-0 [001] ..s. 26879.403122: ehci_hub_status_data:
wgq[ehci_hub_status_data] ignore_oc[0] resuming_ports[1]
<...>-12873 [000] d..1 26879.413379: ehci_hub_control:
[ehci_hub_control]line[907] port[0] write portsc_reg[238014c5]
reset_done[2105769]
<...>-12873 [000] d..1 26879.453173: ehci_hub_control: GetStatus port:1 status
23801885 17 ERR POWER sig=j SUSPEND PE CONNECT
<...>-12873 [000] 26879.473158: check_port_resume_type: port 1 status
.0507 after resume, -19
<...>-12873 [000] 26879.473160: usb_port_resume: status = -19 after
check_port_resume_type
<...>-12873 [000] 26879.473161: usb_port_resume: can't resume, status -19
<...>-12873 [000] 26879.473162: hub_port_logical_disconnect: logical
disconnect on port 1
There is a in-band remote wakeup and controller run in k-state. Then kernel
driver(ClearPortFeature/USB_PORT_FEAT_SUSPEND) write RESUME|LS(k-state) bit
into controller. It makes controller status weird. It's defined in EHCI
controller spec(Revision 1.0), "If it has enabled remote wake-up, a K-state
on the bus will turn the transceiver clock and generate an interrupt. The
software will then have to wait 20 ms for the resume to complete and the
port to go back to an active state." In this case Kernel should wait for
the wakeup finished, then judge what should do next step.
We have some thought and give a patch. This patch is to wait for controller
RESUME finished when hub try to clear port SUSPEND feature.
Signed-off-by: xiao jin
Reviewed-by: David Cohen
---
drivers/usb/host/ehci-hub.c |7 +++
include/linux/usb/ehci_def.h |5 +
2 files changed, 12 insertions(+)
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 7ae0c4d..09a8b6b 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -935,6 +935,13 @@ static int ehci_hub_control (
break;
}
#endif
+ if ((temp & PORT_RESUME)
+ && ((temp & PORT_LS_MASK) == PORT_K_STATE)) {
+ ehci_handshake(ehci, status_reg,
+ PORT_RESUME, 0, 2 /* 20msec */);
+ temp = ehci_readl(ehci, status_reg);
+ temp &= ~PORT_RWC_BITS;
+ }
if (!(temp & PORT_SUSPEND))
break;
if ((temp & PORT_PE) == 0)
diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
index daec99a..0f0f919 100644
--- a/include/linux/usb/ehci_def.h
+++ b/include/linux/usb/ehci_def.h
@@ -149,6 +149,11 @@ struct ehci_regs {
#define PORT_POWER (1<<12) /* true: has power (see PPC) */
#define PORT_USB11(x) (((x)&(3<<10)) == (1<<10)) /* USB 1.1 device */
/* 11:10 for detecting lowspeed devices (reset vs release ownership) */
+#define PORT_LS_MASK (0x3<<10) /* line status */
+#define PORT_SE0_STATE (0<<10)
+#define PORT_K_STATE (1<<10)
+#define PORT_J_STATE (2<<10)
+#define PORT_UNDEFINED_STATE (3<<10)
/* 9 reserved */
#define PORT_LPM (1<<9) /* LPM transaction */
#define PORT_RESET (1<<8) /* reset port */
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] USB: ehci-hub: wait for RESUME finished when hub try to clear SUSPEND
We use usb ehci to connect with modem and run stress test on ehci
remote wake. Sometimes usb disconnect. We add more debug ftrace
(Kernel version: 3.10) and list the key log to show how problem
happened.
idle-0 [000] d.h2 26879.385095: ehci_irq: irq status 1008c PPCE FLR PCD
idle-0 [000] d.h2 26879.385099: ehci_irq: rh_state[2] hcd-state[132]
pstatus[0][238014c5] suspended_ports[1] reset_done[0]
...-12873 [000] d..1 26879.393536: ehci_hub_control: GetStatus port:1 status
238014c5 17 ERR POWER sig=k SUSPEND RESUME PE CONNECT
...-12873 [000] d..1 26879.393549: ehci_hub_control: typeReq [2301] wIndex[1]
wValue[2]
...-12873 [000] d..1 26879.393553: ehci_hub_control:
[ehci_hub_control]line[891] port[0] hostpc_reg [44000202]-[44000202]
idle-0 [001] ..s. 26879.403122: ehci_hub_status_data:
wgq[ehci_hub_status_data] ignore_oc[0] resuming_ports[1]
...-12873 [000] d..1 26879.413379: ehci_hub_control:
[ehci_hub_control]line[907] port[0] write portsc_reg[238014c5]
reset_done[2105769]
...-12873 [000] d..1 26879.453173: ehci_hub_control: GetStatus port:1 status
23801885 17 ERR POWER sig=j SUSPEND PE CONNECT
...-12873 [000] 26879.473158: check_port_resume_type: port 1 status
.0507 after resume, -19
...-12873 [000] 26879.473160: usb_port_resume: status = -19 after
check_port_resume_type
...-12873 [000] 26879.473161: usb_port_resume: can't resume, status -19
...-12873 [000] 26879.473162: hub_port_logical_disconnect: logical
disconnect on port 1
There is a in-band remote wakeup and controller run in k-state. Then kernel
driver(ClearPortFeature/USB_PORT_FEAT_SUSPEND) write RESUME|LS(k-state) bit
into controller. It makes controller status weird. It's defined in EHCI
controller spec(Revision 1.0), If it has enabled remote wake-up, a K-state
on the bus will turn the transceiver clock and generate an interrupt. The
software will then have to wait 20 ms for the resume to complete and the
port to go back to an active state. In this case Kernel should wait for
the wakeup finished, then judge what should do next step.
We have some thought and give a patch. This patch is to wait for controller
RESUME finished when hub try to clear port SUSPEND feature.
Signed-off-by: xiao jin jin.x...@intel.com
Reviewed-by: David Cohen david.a.co...@linux.intel.com
---
drivers/usb/host/ehci-hub.c |7 +++
include/linux/usb/ehci_def.h |5 +
2 files changed, 12 insertions(+)
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 7ae0c4d..09a8b6b 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -935,6 +935,13 @@ static int ehci_hub_control (
break;
}
#endif
+ if ((temp PORT_RESUME)
+((temp PORT_LS_MASK) == PORT_K_STATE)) {
+ ehci_handshake(ehci, status_reg,
+ PORT_RESUME, 0, 2 /* 20msec */);
+ temp = ehci_readl(ehci, status_reg);
+ temp = ~PORT_RWC_BITS;
+ }
if (!(temp PORT_SUSPEND))
break;
if ((temp PORT_PE) == 0)
diff --git a/include/linux/usb/ehci_def.h b/include/linux/usb/ehci_def.h
index daec99a..0f0f919 100644
--- a/include/linux/usb/ehci_def.h
+++ b/include/linux/usb/ehci_def.h
@@ -149,6 +149,11 @@ struct ehci_regs {
#define PORT_POWER (112) /* true: has power (see PPC) */
#define PORT_USB11(x) (((x)(310)) == (110)) /* USB 1.1 device */
/* 11:10 for detecting lowspeed devices (reset vs release ownership) */
+#define PORT_LS_MASK (0x310) /* line status */
+#define PORT_SE0_STATE (010)
+#define PORT_K_STATE (110)
+#define PORT_J_STATE (210)
+#define PORT_UNDEFINED_STATE (310)
/* 9 reserved */
#define PORT_LPM (19) /* LPM transaction */
#define PORT_RESET (18) /* reset port */
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] USB: usb_wwan: fix race between write and resume
We find a race between write and resume. usb_wwan_resume run play_delayed()
and spin_unlock, but intfdata->suspended still is not set to zero.
At this time usb_wwan_write is called and anchor the urb to delay
list. Then resume keep running but the delayed urb have no chance
to be commit until next resume. If the time of next resume is far
away, tty will be blocked in tty_wait_until_sent during time. The
race also can lead to writes being reordered.
This patch put play_Delayed and intfdata->suspended together in the
spinlock, it's to avoid the write race during resume.
Signed-off-by: xiao jin
Signed-off-by: Zhang, Qi1
Reviewed-by: David Cohen
---
drivers/usb/serial/usb_wwan.c |8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index bd30d70..8e06afc 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -663,17 +663,15 @@ int usb_wwan_resume(struct usb_serial *serial)
}
}
+ spin_lock_irq(>susp_lock);
for (i = 0; i < serial->num_ports; i++) {
/* walk all ports */
port = serial->port[i];
portdata = usb_get_serial_port_data(port);
/* skip closed ports */
- spin_lock_irq(>susp_lock);
- if (!portdata || !portdata->opened) {
- spin_unlock_irq(>susp_lock);
+ if (!portdata || !portdata->opened)
continue;
- }
for (j = 0; j < N_IN_URB; j++) {
urb = portdata->in_urbs[j];
@@ -686,9 +684,7 @@ int usb_wwan_resume(struct usb_serial *serial)
}
}
play_delayed(port);
- spin_unlock_irq(>susp_lock);
}
- spin_lock_irq(>susp_lock);
intfdata->suspended = 0;
spin_unlock_irq(>susp_lock);
err_out:
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb_wwan: some improvement on write and resume
They are however two distinct bugs and should be fixed separately. Could you split the fixes into two patches and resubmit? Please also include a more descriptive subject line for each patch, for example: "USB: usb_wwan: fix urb leak in write error path" "USB: usb_wwan: fix race between write and resume" You should probably point out that the write-resume race could also lead to writes being reordered in the description of that patch. Thanks. I will resubmit two patches. Jin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb_wwan: some improvement on write and resume
They are however two distinct bugs and should be fixed separately. Could you split the fixes into two patches and resubmit? Please also include a more descriptive subject line for each patch, for example: USB: usb_wwan: fix urb leak in write error path USB: usb_wwan: fix race between write and resume You should probably point out that the write-resume race could also lead to writes being reordered in the description of that patch. Thanks. I will resubmit two patches. Jin -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] USB: usb_wwan: fix race between write and resume
We find a race between write and resume. usb_wwan_resume run play_delayed()
and spin_unlock, but intfdata-suspended still is not set to zero.
At this time usb_wwan_write is called and anchor the urb to delay
list. Then resume keep running but the delayed urb have no chance
to be commit until next resume. If the time of next resume is far
away, tty will be blocked in tty_wait_until_sent during time. The
race also can lead to writes being reordered.
This patch put play_Delayed and intfdata-suspended together in the
spinlock, it's to avoid the write race during resume.
Signed-off-by: xiao jin jin.x...@intel.com
Signed-off-by: Zhang, Qi1 qi1.zh...@intel.com
Reviewed-by: David Cohen david.a.co...@linux.intel.com
---
drivers/usb/serial/usb_wwan.c |8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index bd30d70..8e06afc 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -663,17 +663,15 @@ int usb_wwan_resume(struct usb_serial *serial)
}
}
+ spin_lock_irq(intfdata-susp_lock);
for (i = 0; i serial-num_ports; i++) {
/* walk all ports */
port = serial-port[i];
portdata = usb_get_serial_port_data(port);
/* skip closed ports */
- spin_lock_irq(intfdata-susp_lock);
- if (!portdata || !portdata-opened) {
- spin_unlock_irq(intfdata-susp_lock);
+ if (!portdata || !portdata-opened)
continue;
- }
for (j = 0; j N_IN_URB; j++) {
urb = portdata-in_urbs[j];
@@ -686,9 +684,7 @@ int usb_wwan_resume(struct usb_serial *serial)
}
}
play_delayed(port);
- spin_unlock_irq(intfdata-susp_lock);
}
- spin_lock_irq(intfdata-susp_lock);
intfdata-suspended = 0;
spin_unlock_irq(intfdata-susp_lock);
err_out:
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb_wwan: some improvement on write and resume
When enable usb serial for modem data, sometimes the tty is blocked
in tty_wait_until_sent because portdata->out_busy always is set and
have no chance to be cleared.
We have found two scenarios lead to portdata->out_busy problem.
1. usb_wwan_write set portdata->out_busy firstly, then try autopm
async with error. No out urb submit and no usb_wwan_outdat_callback
to this write, portdata->out_busy can't be cleared.
2. usb_wwan_resume run play_delayed() and spin_unlock, but
intfdata->suspended still is not set to zero. At this time
usb_wwan_write is called and anchor the urb to delay list. Then resume
keep running but the delayed urb have no chance to be commit until next
resume. If the time of next resume is far away, tty will be blocked
in tty_wait_until_sent during time.
The patch make some enhancement on write and resume.
1. clear portdata->out_busy if usb_wwan_write try autopm async with
error.
2. put play_Delayed and intfdata->suspended together in the spinlock,
it's to avoid the write race during resume.
Signed-off-by: xiao jin
Signed-off-by: Zhang, Qi1
Reviewed-by: David Cohen
---
drivers/usb/serial/usb_wwan.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index 640fe01..8e06afc 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -228,8 +228,10 @@ int usb_wwan_write(struct tty_struct *tty, struct
usb_serial_port *port,
usb_pipeendpoint(this_urb->pipe), i);
err = usb_autopm_get_interface_async(port->serial->interface);
- if (err < 0)
+ if (err < 0) {
+ clear_bit(i, >out_busy);
break;
+ }
/* send the data */
memcpy(this_urb->transfer_buffer, buf, todo);
@@ -661,17 +663,15 @@ int usb_wwan_resume(struct usb_serial *serial)
}
}
+ spin_lock_irq(>susp_lock);
for (i = 0; i < serial->num_ports; i++) {
/* walk all ports */
port = serial->port[i];
portdata = usb_get_serial_port_data(port);
/* skip closed ports */
- spin_lock_irq(>susp_lock);
- if (!portdata || !portdata->opened) {
- spin_unlock_irq(>susp_lock);
+ if (!portdata || !portdata->opened)
continue;
- }
for (j = 0; j < N_IN_URB; j++) {
urb = portdata->in_urbs[j];
@@ -684,9 +684,7 @@ int usb_wwan_resume(struct usb_serial *serial)
}
}
play_delayed(port);
- spin_unlock_irq(>susp_lock);
}
- spin_lock_irq(>susp_lock);
intfdata->suspended = 0;
spin_unlock_irq(>susp_lock);
err_out:
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] inetpeer_gc_worker: trivial cleanup
Do not initialize list twice.
list_replace_init() already takes care of initializing list.
We don't need to initialize it with LIST_HEAD() beforehand.
Signed-off-by: xiao jin
Reviewed-by: David Cohen
---
net/ipv4/inetpeer.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index 48f4244..c98cf14 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -120,7 +120,7 @@ int inet_peer_maxttl __read_mostly = 10 * 60 * HZ; /*
usual time to live: 10 min
static void inetpeer_gc_worker(struct work_struct *work)
{
struct inet_peer *p, *n, *c;
- LIST_HEAD(list);
+ struct list_head list;
spin_lock_bh(_lock);
list_replace_init(_list, );
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] cleanup_net: trivial cleanup
Do not initialize net_kill_list twice.
list_replace_init() already takes care of initializing net_kill_list.
We don't need to initialize it with LIST_HEAD() beforehand.
Signed-off-by: xiao jin
Reviewed-by: David Cohen
---
net/core/net_namespace.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 81d3a9a..05e949d 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -273,7 +273,7 @@ static void cleanup_net(struct work_struct *work)
{
const struct pernet_operations *ops;
struct net *net, *tmp;
- LIST_HEAD(net_kill_list);
+ struct list_head net_kill_list;
LIST_HEAD(net_exit_list);
/* Atomically snapshot the list of namespaces to cleanup */
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] cleanup_net: trivial cleanup
Do not initialize net_kill_list twice.
list_replace_init() already takes care of initializing net_kill_list.
We don't need to initialize it with LIST_HEAD() beforehand.
Signed-off-by: xiao jin jin.x...@intel.com
Reviewed-by: David Cohen david.a.co...@linux.intel.com
---
net/core/net_namespace.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 81d3a9a..05e949d 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -273,7 +273,7 @@ static void cleanup_net(struct work_struct *work)
{
const struct pernet_operations *ops;
struct net *net, *tmp;
- LIST_HEAD(net_kill_list);
+ struct list_head net_kill_list;
LIST_HEAD(net_exit_list);
/* Atomically snapshot the list of namespaces to cleanup */
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] inetpeer_gc_worker: trivial cleanup
Do not initialize list twice.
list_replace_init() already takes care of initializing list.
We don't need to initialize it with LIST_HEAD() beforehand.
Signed-off-by: xiao jin jin.x...@intel.com
Reviewed-by: David Cohen david.a.co...@linux.intel.com
---
net/ipv4/inetpeer.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index 48f4244..c98cf14 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -120,7 +120,7 @@ int inet_peer_maxttl __read_mostly = 10 * 60 * HZ; /*
usual time to live: 10 min
static void inetpeer_gc_worker(struct work_struct *work)
{
struct inet_peer *p, *n, *c;
- LIST_HEAD(list);
+ struct list_head list;
spin_lock_bh(gc_lock);
list_replace_init(gc_list, list);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb_wwan: some improvement on write and resume
When enable usb serial for modem data, sometimes the tty is blocked
in tty_wait_until_sent because portdata-out_busy always is set and
have no chance to be cleared.
We have found two scenarios lead to portdata-out_busy problem.
1. usb_wwan_write set portdata-out_busy firstly, then try autopm
async with error. No out urb submit and no usb_wwan_outdat_callback
to this write, portdata-out_busy can't be cleared.
2. usb_wwan_resume run play_delayed() and spin_unlock, but
intfdata-suspended still is not set to zero. At this time
usb_wwan_write is called and anchor the urb to delay list. Then resume
keep running but the delayed urb have no chance to be commit until next
resume. If the time of next resume is far away, tty will be blocked
in tty_wait_until_sent during time.
The patch make some enhancement on write and resume.
1. clear portdata-out_busy if usb_wwan_write try autopm async with
error.
2. put play_Delayed and intfdata-suspended together in the spinlock,
it's to avoid the write race during resume.
Signed-off-by: xiao jin jin.x...@intel.com
Signed-off-by: Zhang, Qi1 qi1.zh...@intel.com
Reviewed-by: David Cohen david.a.co...@linux.intel.com
---
drivers/usb/serial/usb_wwan.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index 640fe01..8e06afc 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -228,8 +228,10 @@ int usb_wwan_write(struct tty_struct *tty, struct
usb_serial_port *port,
usb_pipeendpoint(this_urb-pipe), i);
err = usb_autopm_get_interface_async(port-serial-interface);
- if (err 0)
+ if (err 0) {
+ clear_bit(i, portdata-out_busy);
break;
+ }
/* send the data */
memcpy(this_urb-transfer_buffer, buf, todo);
@@ -661,17 +663,15 @@ int usb_wwan_resume(struct usb_serial *serial)
}
}
+ spin_lock_irq(intfdata-susp_lock);
for (i = 0; i serial-num_ports; i++) {
/* walk all ports */
port = serial-port[i];
portdata = usb_get_serial_port_data(port);
/* skip closed ports */
- spin_lock_irq(intfdata-susp_lock);
- if (!portdata || !portdata-opened) {
- spin_unlock_irq(intfdata-susp_lock);
+ if (!portdata || !portdata-opened)
continue;
- }
for (j = 0; j N_IN_URB; j++) {
urb = portdata-in_urbs[j];
@@ -684,9 +684,7 @@ int usb_wwan_resume(struct usb_serial *serial)
}
}
play_delayed(port);
- spin_unlock_irq(intfdata-susp_lock);
}
- spin_lock_irq(intfdata-susp_lock);
intfdata-suspended = 0;
spin_unlock_irq(intfdata-susp_lock);
err_out:
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: cdc-acm: fix broken runtime suspend
Hi, Johan,
On 04/15/2014 03:58 AM, Johan Hovold wrote:
The current ACM runtime-suspend implementation is broken in several
ways:
Firstly, it buffers only the first write request being made while
suspended -- any further writes are silently dropped.
Secondly, writes being dropped also leak write urbs, which are never
reclaimed (until the device is unbound).
Thirdly, even the single buffered write is not cleared at shutdown
(which may happen before the device is resumed), something which can
lead to another urb leak as well as a PM usage-counter leak.
Fix this by implementing a delayed-write queue using urb anchors and
making sure to discard the queue properly at shutdown.
Reported-by: Xiao Jin
Signed-off-by: Johan Hovold
---
Let's fix the current runtime-suspend implementation before considering
adding a write fifo.
Since this has never really worked, I'll let someone else decide whether
the fix should be back-ported to stable or not.
Jin, did you check what closing_wait setting your application is using?
I check the closing_wait is 30s by default. Below is the trace we get
when reproduced problem.
<...>-1360 [003] d.s5 1843.061418: acm_tty_write:
acm_tty_write - write 65
<...>-1360 [003] d.s5 1843.061425: acm_write_start:
acm_write_start - susp_count 2
<...>-2535 [002] 1843.180687: acm_tty_close:
acm_tty_close
<...>-2535 [002] 1843.181217: acm_wb_is_avail: avail n=15
<...>-2535 [002] 1843.181238: acm_port_shutdown:
acm_port_shutdown
<...>-438 [003] 1843.182803: acm_wb_is_avail: avail n=16
<...>-438 [003] d..1 1843.182817: acm_tty_write:
acm_tty_write - write 11
<...>-438 [003] d..1 1843.182826: acm_write_start:
acm_write_start - susp_count 2
<...>-37[003] 1843.202884: acm_resume: wgq[acm_resume]
<...>-37[003] 1843.202892: acm_resume: wgq[acm_resume]
<...>-37[003] d..1 1843.203195: acm_resume: send
d_wb-1046297992
<...>-37[003] 1843.203199: acm_start_wb:
acm_start_wb, acm->transmitting=0
-0 [000] d.h2 1843.203343: acm_write_done.isra.11:
acm_write_done, acm->transmitting=1
<...>-1989 [001] 1843.207197: acm_tty_cleanup:
acm_tty_cleanup
There are two acms in the case, acm0 and acm3. acm0 have delayed 65
bytes before close. When acm0 close, ASYNCB_INITIALIZED flag is cleared,
that lead to acm0 have no chance to start delayed wb during acm resume.
acm3 delayed 11 bytes send out because it still is opened.
It looks closing_wait didn't take effect at that time. I am not sure the
reason why because we have no more debug log. Now We are checking the
issue again.
Could you give this patch a try as well?
I try the write and resume part of this patch, anchor urb works well.
Thanks,
Johan
drivers/usb/class/cdc-acm.c | 36 +++-
drivers/usb/class/cdc-acm.h | 2 +-
2 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 900f7ff805ee..ebbcc7a6a7c8 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -571,6 +571,8 @@ static void acm_port_destruct(struct tty_port *port)
static void acm_port_shutdown(struct tty_port *port)
{
struct acm *acm = container_of(port, struct acm, port);
+ struct urb *urb;
+ struct acm_wb *wb;
int i;
dev_dbg(>control->dev, "%s\n", __func__);
@@ -578,6 +580,16 @@ static void acm_port_shutdown(struct tty_port *port)
mutex_lock(>mutex);
if (!acm->disconnected) {
usb_autopm_get_interface(acm->control);
+ spin_lock_irq(>write_lock);
+ for (;;) {
+ urb = usb_get_from_anchor(>delayed);
+ if (!urb)
+ break;
+ wb = urb->context;
+ wb->use = 0;
+ usb_autopm_put_interface_async(acm->control);
+ }
+ spin_unlock_irq(>write_lock);
acm_set_control(acm, acm->ctrlout = 0);
usb_kill_urb(acm->ctrlurb);
for (i = 0; i < ACM_NW; i++)
@@ -646,12 +658,9 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm->control);
if (acm->susp_count) {
- if (!acm->delayed_wb)
- acm->delayed_wb = wb;
- else
- usb_autopm_put_interface_async(acm->control);
+ usb_anchor_urb(wb->urb, >delayed);
spin_unlock_irqrestore(>write_lock, flags);
- return count; /* A white lie */
+
Re: [PATCH] USB: cdc-acm: fix broken runtime suspend
Hi, Johan,
On 04/15/2014 03:58 AM, Johan Hovold wrote:
The current ACM runtime-suspend implementation is broken in several
ways:
Firstly, it buffers only the first write request being made while
suspended -- any further writes are silently dropped.
Secondly, writes being dropped also leak write urbs, which are never
reclaimed (until the device is unbound).
Thirdly, even the single buffered write is not cleared at shutdown
(which may happen before the device is resumed), something which can
lead to another urb leak as well as a PM usage-counter leak.
Fix this by implementing a delayed-write queue using urb anchors and
making sure to discard the queue properly at shutdown.
Reported-by: Xiao Jin jin.x...@intel.com
Signed-off-by: Johan Hovold jhov...@gmail.com
---
Let's fix the current runtime-suspend implementation before considering
adding a write fifo.
Since this has never really worked, I'll let someone else decide whether
the fix should be back-ported to stable or not.
Jin, did you check what closing_wait setting your application is using?
I check the closing_wait is 30s by default. Below is the trace we get
when reproduced problem.
...-1360 [003] d.s5 1843.061418: acm_tty_write:
acm_tty_write - write 65
...-1360 [003] d.s5 1843.061425: acm_write_start:
acm_write_start - susp_count 2
...-2535 [002] 1843.180687: acm_tty_close:
acm_tty_close
...-2535 [002] 1843.181217: acm_wb_is_avail: avail n=15
...-2535 [002] 1843.181238: acm_port_shutdown:
acm_port_shutdown
...-438 [003] 1843.182803: acm_wb_is_avail: avail n=16
...-438 [003] d..1 1843.182817: acm_tty_write:
acm_tty_write - write 11
...-438 [003] d..1 1843.182826: acm_write_start:
acm_write_start - susp_count 2
...-37[003] 1843.202884: acm_resume: wgq[acm_resume]
...-37[003] 1843.202892: acm_resume: wgq[acm_resume]
...-37[003] d..1 1843.203195: acm_resume: send
d_wb-1046297992
...-37[003] 1843.203199: acm_start_wb:
acm_start_wb, acm-transmitting=0
idle-0 [000] d.h2 1843.203343: acm_write_done.isra.11:
acm_write_done, acm-transmitting=1
...-1989 [001] 1843.207197: acm_tty_cleanup:
acm_tty_cleanup
There are two acms in the case, acm0 and acm3. acm0 have delayed 65
bytes before close. When acm0 close, ASYNCB_INITIALIZED flag is cleared,
that lead to acm0 have no chance to start delayed wb during acm resume.
acm3 delayed 11 bytes send out because it still is opened.
It looks closing_wait didn't take effect at that time. I am not sure the
reason why because we have no more debug log. Now We are checking the
issue again.
Could you give this patch a try as well?
I try the write and resume part of this patch, anchor urb works well.
Thanks,
Johan
drivers/usb/class/cdc-acm.c | 36 +++-
drivers/usb/class/cdc-acm.h | 2 +-
2 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 900f7ff805ee..ebbcc7a6a7c8 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -571,6 +571,8 @@ static void acm_port_destruct(struct tty_port *port)
static void acm_port_shutdown(struct tty_port *port)
{
struct acm *acm = container_of(port, struct acm, port);
+ struct urb *urb;
+ struct acm_wb *wb;
int i;
dev_dbg(acm-control-dev, %s\n, __func__);
@@ -578,6 +580,16 @@ static void acm_port_shutdown(struct tty_port *port)
mutex_lock(acm-mutex);
if (!acm-disconnected) {
usb_autopm_get_interface(acm-control);
+ spin_lock_irq(acm-write_lock);
+ for (;;) {
+ urb = usb_get_from_anchor(acm-delayed);
+ if (!urb)
+ break;
+ wb = urb-context;
+ wb-use = 0;
+ usb_autopm_put_interface_async(acm-control);
+ }
+ spin_unlock_irq(acm-write_lock);
acm_set_control(acm, acm-ctrlout = 0);
usb_kill_urb(acm-ctrlurb);
for (i = 0; i ACM_NW; i++)
@@ -646,12 +658,9 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm-control);
if (acm-susp_count) {
- if (!acm-delayed_wb)
- acm-delayed_wb = wb;
- else
- usb_autopm_put_interface_async(acm-control);
+ usb_anchor_urb(wb-urb, acm-delayed);
spin_unlock_irqrestore(acm-write_lock, flags);
- return count; /* A white lie */
+ return count;
}
usb_mark_last_busy(acm-dev);
@@ -1267,6 +1276,7 @@ made_compressed_probe:
acm-bInterval = epread
Re: [PATCH] cdc-acm: some enhancement on acm delayed write
Hi, Oliver, On 04/10/2014 04:02 PM, Oliver Neukum wrote: On Wed, 2014-04-09 at 22:57 +0800, Xiao Jin wrote: Thanks all for the review. We meet with the problems when developing product. I would like to explain my understanding. On 04/08/2014 11:05 AM, Xiao Jin wrote: We find two problems on acm tty write delayed mechanism. (1) When acm resume, the delayed wb will be started. But now only one write can be saved during acm suspend. More acm write may be abandoned. The scenario usually happened when user space write series AT after acm suspend. If acm accept the first AT, what's the reason for acm to refuse the second AT? If write return 0, user space will try repeatedly until resume. It looks simpler that acm accept all the data and sent out urb when resume. No. We cannot accept an arbitrary amount of data. It would let any user OOM the system. There will have to be an arbitrary limit. The simplest limit is 1 urb. And that is because we said that we are ready to accept data. We apply cdc-acm for modem AT data. I can find other usb modem driver usb_wwan_write use list to accept more data when suspend, maybe usbnet is the same. Do you have any more reason for me to understand why cdc-acm accept only one? (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when close. If acm resume callback run after ASYNCB_INITIALIZED flag cleared, there will have no chance for delayed write to start. That lead to acm_wb.use can't be cleared. If user space open acm tty again and try to setd, tty will be blocked in tty_wait_until_sent for ever. We see tty write and close concurrently after acm suspend in this case. It looks no method to avoid it from tty layer. acm_tty_write and There is a delay user space can set. acm_resume call after acm_port_shutdown. It looks any action in acm_port_shutdown can't solve the problem. As acm has accepted the user space data, we can only find a way to send out urb. I feel anyway to discard the data looks like a lie to user space. In my understanding acm should accept data as much as possible, and send out urb as soon as possible. What do you think of? There's certainly no problem with sending out the data. Yet simply resuming the device in shutdown() should do the job. We see tty write and close concurrently, we have debug log to show that acm_tty_write and acm_resume is called after acm_port_shutdown, I don't understand "resuming the device in shutdown() should do the job". Regards Oliver My understanding may be superficial, please correct me if anything wrong. Thanks. Br, Jin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cdc-acm: some enhancement on acm delayed write
Hi, Oliver, On 04/10/2014 04:02 PM, Oliver Neukum wrote: On Wed, 2014-04-09 at 22:57 +0800, Xiao Jin wrote: Thanks all for the review. We meet with the problems when developing product. I would like to explain my understanding. On 04/08/2014 11:05 AM, Xiao Jin wrote: We find two problems on acm tty write delayed mechanism. (1) When acm resume, the delayed wb will be started. But now only one write can be saved during acm suspend. More acm write may be abandoned. The scenario usually happened when user space write series AT after acm suspend. If acm accept the first AT, what's the reason for acm to refuse the second AT? If write return 0, user space will try repeatedly until resume. It looks simpler that acm accept all the data and sent out urb when resume. No. We cannot accept an arbitrary amount of data. It would let any user OOM the system. There will have to be an arbitrary limit. The simplest limit is 1 urb. And that is because we said that we are ready to accept data. We apply cdc-acm for modem AT data. I can find other usb modem driver usb_wwan_write use list to accept more data when suspend, maybe usbnet is the same. Do you have any more reason for me to understand why cdc-acm accept only one? (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when close. If acm resume callback run after ASYNCB_INITIALIZED flag cleared, there will have no chance for delayed write to start. That lead to acm_wb.use can't be cleared. If user space open acm tty again and try to setd, tty will be blocked in tty_wait_until_sent for ever. We see tty write and close concurrently after acm suspend in this case. It looks no method to avoid it from tty layer. acm_tty_write and There is a delay user space can set. acm_resume call after acm_port_shutdown. It looks any action in acm_port_shutdown can't solve the problem. As acm has accepted the user space data, we can only find a way to send out urb. I feel anyway to discard the data looks like a lie to user space. In my understanding acm should accept data as much as possible, and send out urb as soon as possible. What do you think of? There's certainly no problem with sending out the data. Yet simply resuming the device in shutdown() should do the job. We see tty write and close concurrently, we have debug log to show that acm_tty_write and acm_resume is called after acm_port_shutdown, I don't understand resuming the device in shutdown() should do the job. Regards Oliver My understanding may be superficial, please correct me if anything wrong. Thanks. Br, Jin -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cdc-acm: some enhancement on acm delayed write
Thanks all for the review. We meet with the problems when developing product. I would like to explain my understanding. On 04/08/2014 11:05 AM, Xiao Jin wrote: We find two problems on acm tty write delayed mechanism. (1) When acm resume, the delayed wb will be started. But now only one write can be saved during acm suspend. More acm write may be abandoned. The scenario usually happened when user space write series AT after acm suspend. If acm accept the first AT, what's the reason for acm to refuse the second AT? If write return 0, user space will try repeatedly until resume. It looks simpler that acm accept all the data and sent out urb when resume. (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when close. If acm resume callback run after ASYNCB_INITIALIZED flag cleared, there will have no chance for delayed write to start. That lead to acm_wb.use can't be cleared. If user space open acm tty again and try to setd, tty will be blocked in tty_wait_until_sent for ever. We see tty write and close concurrently after acm suspend in this case. It looks no method to avoid it from tty layer. acm_tty_write and acm_resume call after acm_port_shutdown. It looks any action in acm_port_shutdown can't solve the problem. As acm has accepted the user space data, we can only find a way to send out urb. I feel anyway to discard the data looks like a lie to user space. In my understanding acm should accept data as much as possible, and send out urb as soon as possible. What do you think of? Br, Jin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cdc-acm: some enhancement on acm delayed write
Thanks all for the review. We meet with the problems when developing product. I would like to explain my understanding. On 04/08/2014 11:05 AM, Xiao Jin wrote: We find two problems on acm tty write delayed mechanism. (1) When acm resume, the delayed wb will be started. But now only one write can be saved during acm suspend. More acm write may be abandoned. The scenario usually happened when user space write series AT after acm suspend. If acm accept the first AT, what's the reason for acm to refuse the second AT? If write return 0, user space will try repeatedly until resume. It looks simpler that acm accept all the data and sent out urb when resume. (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when close. If acm resume callback run after ASYNCB_INITIALIZED flag cleared, there will have no chance for delayed write to start. That lead to acm_wb.use can't be cleared. If user space open acm tty again and try to setd, tty will be blocked in tty_wait_until_sent for ever. We see tty write and close concurrently after acm suspend in this case. It looks no method to avoid it from tty layer. acm_tty_write and acm_resume call after acm_port_shutdown. It looks any action in acm_port_shutdown can't solve the problem. As acm has accepted the user space data, we can only find a way to send out urb. I feel anyway to discard the data looks like a lie to user space. In my understanding acm should accept data as much as possible, and send out urb as soon as possible. What do you think of? Br, Jin -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] cdc-acm: some enhancement on acm delayed write
From: xiao jin
Date: Tue, 25 Mar 2014 15:54:36 +0800
Subject: [PATCH] cdc-acm: some enhancement on acm delayed write
We find two problems on acm tty write delayed mechanism.
(1) When acm resume, the delayed wb will be started. But now
only one write can be saved during acm suspend. More acm write
may be abandoned.
(2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
close. If acm resume callback run after ASYNCB_INITIALIZED flag
cleared, there will have no chance for delayed write to start.
That lead to acm_wb.use can't be cleared. If user space open
acm tty again and try to setd, tty will be blocked in
tty_wait_until_sent for ever.
This patch have two modification.
(1) use list_head to save the write acm_wb during acm suspend.
It can ensure no acm write abandoned.
(2) enable flush buffer callback when acm tty close. acm delayed
wb will start before acm port shutdown callback, it make sure
the delayed acm_wb.use to be cleared. The patch also clear
acm_wb.use and acm.transmitting when port shutdown.
Signed-off-by: xiao jin
Reviewed-by: David Cohen
---
drivers/usb/class/cdc-acm.c | 56
++-
drivers/usb/class/cdc-acm.h |3 ++-
2 files changed, 42 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 900f7ff..cfe00b2 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -217,6 +217,24 @@ static int acm_start_wb(struct acm *acm, struct
acm_wb *wb)
}
/*
+ * Delayed write.
+ */
+static int acm_start_delayed_wb(struct acm *acm)
+{
+ struct acm_wb *wb, *n_wb;
+ LIST_HEAD(delay_wb_list);
+
+ spin_lock_irq(>write_lock);
+ list_replace_init(>delayed_wb_list, _wb_list);
+ spin_unlock_irq(>write_lock);
+ list_for_each_entry_safe(wb, n_wb,
+ _wb_list, delay) {
+ list_del(>delay);
+ acm_start_wb(acm, wb);
+ }
+}
+
+/*
* attributes exported through sysfs
*/
static ssize_t show_caps
@@ -580,8 +598,11 @@ static void acm_port_shutdown(struct tty_port *port)
usb_autopm_get_interface(acm->control);
acm_set_control(acm, acm->ctrlout = 0);
usb_kill_urb(acm->ctrlurb);
- for (i = 0; i < ACM_NW; i++)
+ acm->transmitting = 0;
+ for (i = 0; i < ACM_NW; i++) {
usb_kill_urb(acm->wb[i].urb);
+ acm->wb[i].use = 0;
+ }
for (i = 0; i < acm->rx_buflimit; i++)
usb_kill_urb(acm->read_urbs[i]);
acm->control->needs_remote_wakeup = 0;
@@ -608,6 +629,8 @@ static void acm_tty_close(struct tty_struct *tty,
struct file *filp)
{
struct acm *acm = tty->driver_data;
dev_dbg(>control->dev, "%s\n", __func__);
+ /* Set flow_stopped to enable flush buffer*/
+ tty->flow_stopped = 1;
tty_port_close(>port, tty, filp);
}
@@ -646,10 +669,7 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm->control);
if (acm->susp_count) {
- if (!acm->delayed_wb)
- acm->delayed_wb = wb;
- else
- usb_autopm_put_interface_async(acm->control);
+ list_add_tail(>delay, >delayed_wb_list);
spin_unlock_irqrestore(>write_lock, flags);
return count; /* A white lie */
}
@@ -688,6 +708,18 @@ static int acm_tty_chars_in_buffer(struct
tty_struct *tty)
return (ACM_NW - acm_wb_is_avail(acm)) * acm->writesize;
}
+static void acm_tty_flush_buffer(struct tty_struct *tty)
+{
+ struct acm *acm = tty->driver_data;
+
+ /* flush delayed write buffer */
+ if (!acm->disconnected) {
+ usb_autopm_get_interface(acm->control);
+ acm_start_delayed_wb(acm);
+ usb_autopm_put_interface(acm->control);
+ }
+}
+
static void acm_tty_throttle(struct tty_struct *tty)
{
struct acm *acm = tty->driver_data;
@@ -1346,6 +1378,7 @@ made_compressed_probe:
snd->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
snd->instance = acm;
}
+ INIT_LIST_HEAD(>delayed_wb_list);
usb_set_intfdata(intf, acm);
@@ -1540,7 +1573,6 @@ static int acm_suspend(struct usb_interface *intf,
pm_message_t message)
static int acm_resume(struct usb_interface *intf)
{
struct acm *acm = usb_get_intfdata(intf);
- struct acm_wb *wb;
int rv = 0;
int cnt;
@@ -1555,16 +1587,7 @@ static int acm_resume(struct usb_interface *intf)
if (test_bit(ASYNCB_INITIALIZED, >port.flags)) {
rv = usb_submit_urb(acm->ctrlurb, GFP_NOIO);
- spin_lock_irq(>write
[PATCH] cdc-acm: some enhancement on acm delayed write
From: xiao jin jin.x...@intel.com
Date: Tue, 25 Mar 2014 15:54:36 +0800
Subject: [PATCH] cdc-acm: some enhancement on acm delayed write
We find two problems on acm tty write delayed mechanism.
(1) When acm resume, the delayed wb will be started. But now
only one write can be saved during acm suspend. More acm write
may be abandoned.
(2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
close. If acm resume callback run after ASYNCB_INITIALIZED flag
cleared, there will have no chance for delayed write to start.
That lead to acm_wb.use can't be cleared. If user space open
acm tty again and try to setd, tty will be blocked in
tty_wait_until_sent for ever.
This patch have two modification.
(1) use list_head to save the write acm_wb during acm suspend.
It can ensure no acm write abandoned.
(2) enable flush buffer callback when acm tty close. acm delayed
wb will start before acm port shutdown callback, it make sure
the delayed acm_wb.use to be cleared. The patch also clear
acm_wb.use and acm.transmitting when port shutdown.
Signed-off-by: xiao jin jin.x...@intel.com
Reviewed-by: David Cohen david.a.co...@linux.intel.com
---
drivers/usb/class/cdc-acm.c | 56
++-
drivers/usb/class/cdc-acm.h |3 ++-
2 files changed, 42 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 900f7ff..cfe00b2 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -217,6 +217,24 @@ static int acm_start_wb(struct acm *acm, struct
acm_wb *wb)
}
/*
+ * Delayed write.
+ */
+static int acm_start_delayed_wb(struct acm *acm)
+{
+ struct acm_wb *wb, *n_wb;
+ LIST_HEAD(delay_wb_list);
+
+ spin_lock_irq(acm-write_lock);
+ list_replace_init(acm-delayed_wb_list, delay_wb_list);
+ spin_unlock_irq(acm-write_lock);
+ list_for_each_entry_safe(wb, n_wb,
+ delay_wb_list, delay) {
+ list_del(wb-delay);
+ acm_start_wb(acm, wb);
+ }
+}
+
+/*
* attributes exported through sysfs
*/
static ssize_t show_caps
@@ -580,8 +598,11 @@ static void acm_port_shutdown(struct tty_port *port)
usb_autopm_get_interface(acm-control);
acm_set_control(acm, acm-ctrlout = 0);
usb_kill_urb(acm-ctrlurb);
- for (i = 0; i ACM_NW; i++)
+ acm-transmitting = 0;
+ for (i = 0; i ACM_NW; i++) {
usb_kill_urb(acm-wb[i].urb);
+ acm-wb[i].use = 0;
+ }
for (i = 0; i acm-rx_buflimit; i++)
usb_kill_urb(acm-read_urbs[i]);
acm-control-needs_remote_wakeup = 0;
@@ -608,6 +629,8 @@ static void acm_tty_close(struct tty_struct *tty,
struct file *filp)
{
struct acm *acm = tty-driver_data;
dev_dbg(acm-control-dev, %s\n, __func__);
+ /* Set flow_stopped to enable flush buffer*/
+ tty-flow_stopped = 1;
tty_port_close(acm-port, tty, filp);
}
@@ -646,10 +669,7 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm-control);
if (acm-susp_count) {
- if (!acm-delayed_wb)
- acm-delayed_wb = wb;
- else
- usb_autopm_put_interface_async(acm-control);
+ list_add_tail(wb-delay, acm-delayed_wb_list);
spin_unlock_irqrestore(acm-write_lock, flags);
return count; /* A white lie */
}
@@ -688,6 +708,18 @@ static int acm_tty_chars_in_buffer(struct
tty_struct *tty)
return (ACM_NW - acm_wb_is_avail(acm)) * acm-writesize;
}
+static void acm_tty_flush_buffer(struct tty_struct *tty)
+{
+ struct acm *acm = tty-driver_data;
+
+ /* flush delayed write buffer */
+ if (!acm-disconnected) {
+ usb_autopm_get_interface(acm-control);
+ acm_start_delayed_wb(acm);
+ usb_autopm_put_interface(acm-control);
+ }
+}
+
static void acm_tty_throttle(struct tty_struct *tty)
{
struct acm *acm = tty-driver_data;
@@ -1346,6 +1378,7 @@ made_compressed_probe:
snd-urb-transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
snd-instance = acm;
}
+ INIT_LIST_HEAD(acm-delayed_wb_list);
usb_set_intfdata(intf, acm);
@@ -1540,7 +1573,6 @@ static int acm_suspend(struct usb_interface *intf,
pm_message_t message)
static int acm_resume(struct usb_interface *intf)
{
struct acm *acm = usb_get_intfdata(intf);
- struct acm_wb *wb;
int rv = 0;
int cnt;
@@ -1555,16 +1587,7 @@ static int acm_resume(struct usb_interface *intf)
if (test_bit(ASYNCB_INITIALIZED, acm-port.flags)) {
rv = usb_submit_urb(acm-ctrlurb, GFP_NOIO);
- spin_lock_irq(acm-write_lock);
- if (acm-delayed_wb
[PATCH] xhci-ring: process bulk set actual_length only when not COMP_STOP_INVAL
When suspend, some urb is put into cancelled_td_list.
process_bulk_intr_td may process the last trb as below:
xhci_transfer_event.transfer_len = 0
xhci_transfer_event.trb_comp_code = 27
urb.transfer_buffer_length = 1024
trb_comp_code is COMP_STOP_INVAL, it should be taken as
invalid urb and discarded, but urb actual_length still is
calculated as transfer_buffer_length. When handling Stop
Endpoint Command completion, the urb in cancelled_td_list
is transferred to upper layer(cdc-acm for example). The
content of urb transfer_buffer is the overlay of previous
two read transfer_buffer. It makes upper layer process wrong
buffer.
This patch is to set actual_length as transfer_buffer_length
only if trb_comp_code is not COMP_STOP_INVAL when
process_bulk_intr_td.
Signed-off-by: xiao jin
---
drivers/usb/host/xhci-ring.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6bfbd80..c9a8863 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2319,8 +2319,11 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci,
struct xhci_td *td,
*status = 0;
}
} else {
- td->urb->actual_length =
- td->urb->transfer_buffer_length;
+ if (trb_comp_code != COMP_STOP_INVAL)
+ td->urb->actual_length =
+ td->urb->transfer_buffer_length;
+ else
+ td->urb->actual_length = 0;
/* Ignore a short packet completion if the
* untransferred length was zero.
*/
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] xhci-ring: process bulk set actual_length only when not COMP_STOP_INVAL
When suspend, some urb is put into cancelled_td_list.
process_bulk_intr_td may process the last trb as below:
xhci_transfer_event.transfer_len = 0
xhci_transfer_event.trb_comp_code = 27
urb.transfer_buffer_length = 1024
trb_comp_code is COMP_STOP_INVAL, it should be taken as
invalid urb and discarded, but urb actual_length still is
calculated as transfer_buffer_length. When handling Stop
Endpoint Command completion, the urb in cancelled_td_list
is transferred to upper layer(cdc-acm for example). The
content of urb transfer_buffer is the overlay of previous
two read transfer_buffer. It makes upper layer process wrong
buffer.
This patch is to set actual_length as transfer_buffer_length
only if trb_comp_code is not COMP_STOP_INVAL when
process_bulk_intr_td.
Signed-off-by: xiao jin jin.x...@intel.com
---
drivers/usb/host/xhci-ring.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6bfbd80..c9a8863 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2319,8 +2319,11 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci,
struct xhci_td *td,
*status = 0;
}
} else {
- td-urb-actual_length =
- td-urb-transfer_buffer_length;
+ if (trb_comp_code != COMP_STOP_INVAL)
+ td-urb-actual_length =
+ td-urb-transfer_buffer_length;
+ else
+ td-urb-actual_length = 0;
/* Ignore a short packet completion if the
* untransferred length was zero.
*/
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] USB: cdc-acm: put delayed_wb to list
Hi, Greg, I am sorry for the inconvenience again. I will do as what you point to make sure the thing won't happen again in future. On Wed, 2013-10-16 at 13:44 -0700, Greg KH wrote: > On Tue, Oct 15, 2013 at 09:01:35AM +0800, xiao jin wrote: > > If acm_write_start during acm suspend, write acm_wb is backuped > > to delayed_wb. When acm resume, the delayed_wb will be started. > > This mechanism can only record one write during acm suspend. More > > acm write will be abandoned. > > > > This patch is to use list_head to record the write acm_wb during acm > > suspend. It can ensure no acm write abandoned. > > > > Signed-off-by: xiao jin > > You obviously did not test this patch at all, as it breaks the build. > > Please get a senior Intel kernel developer to sign-off on your future > patches so this does not happen again. > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] USB: cdc-acm: put delayed_wb to list
Hi, Greg, I am sorry for the inconvenience again. I will do as what you point to make sure the thing won't happen again in future. On Wed, 2013-10-16 at 13:44 -0700, Greg KH wrote: On Tue, Oct 15, 2013 at 09:01:35AM +0800, xiao jin wrote: If acm_write_start during acm suspend, write acm_wb is backuped to delayed_wb. When acm resume, the delayed_wb will be started. This mechanism can only record one write during acm suspend. More acm write will be abandoned. This patch is to use list_head to record the write acm_wb during acm suspend. It can ensure no acm write abandoned. Signed-off-by: xiao jin jin.x...@intel.com You obviously did not test this patch at all, as it breaks the build. Please get a senior Intel kernel developer to sign-off on your future patches so this does not happen again. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] cdc-acm: put delayed_wb to list
If acm_write_start during acm suspend, write acm_wb is backuped
to delayed_wb. When acm resume, the delayed_wb will be started.
This mechanism can only record one write during acm suspend. More
acm write will be abandoned.
This patch is to use list_head to record the write acm_wb during acm
suspend. It can ensure no acm write abandoned.
Signed-off-by: xiao jin
---
drivers/usb/class/cdc-acm.c | 30 --
drivers/usb/class/cdc-acm.h |7 ++-
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 9f49bfe..be679be 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -607,6 +607,7 @@ static int acm_tty_write(struct tty_struct *tty,
unsigned long flags;
int wbn;
struct acm_wb *wb;
+ struct delayed_wb *d_wb;
if (!count)
return 0;
@@ -634,12 +635,17 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm->control);
if (acm->susp_count) {
- if (!acm->delayed_wb)
- acm->delayed_wb = wb;
- else
+ d_wb = kmalloc(sizeof(struct delayed_wb), GFP_ATOMIC);
+ if (d_wb == NULL) {
usb_autopm_put_interface_async(acm->control);
- spin_unlock_irqrestore(>write_lock, flags);
- return count; /* A white lie */
+ spin_unlock_irqrestore(>write_lock, flags);
+ return -ENOMEM;
+ } else {
+ d_wb->wb = wb;
+ list_add_tail(_wb->list, >delayed_wb_list);
+ spin_unlock_irqrestore(>write_lock, flags);
+ return count; /* A white lie */
+ }
}
usb_mark_last_busy(acm->dev);
@@ -1255,6 +1261,7 @@ made_compressed_probe:
snd->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
snd->instance = acm;
}
+ INIT_LIST_HEAD(>delayed_wb_list);
usb_set_intfdata(intf, acm);
@@ -1449,6 +1456,7 @@ static int acm_resume(struct usb_interface *intf)
{
struct acm *acm = usb_get_intfdata(intf);
struct acm_wb *wb;
+ struct delayed_wb *d_wb, *nd_wb;
int rv = 0;
int cnt;
@@ -1464,14 +1472,16 @@ static int acm_resume(struct usb_interface *intf)
rv = usb_submit_urb(acm->ctrlurb, GFP_NOIO);
spin_lock_irq(>write_lock);
- if (acm->delayed_wb) {
- wb = acm->delayed_wb;
- acm->delayed_wb = NULL;
+ list_for_each_entry_safe(d_wb, nd_wb,
+ >delayed_wb_list, list) {
+ wb = d_wb->wb;
+ list_del(_wb->list);
+ kfree(d_wb);
spin_unlock_irq(>write_lock);
acm_start_wb(acm, wb);
- } else {
- spin_unlock_irq(>write_lock);
+ spin_lock_irq(>write_lock);
}
+ spin_unlock_irq(>write_lock);
/*
* delayed error checking because we must
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index 0f76e4a..5eed93f 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -79,6 +79,11 @@ struct acm_rb {
struct acm *instance;
};
+struct delayed_wb {
+ struct list_headlist;
+ struct acm_wb *wb;
+}
+
struct acm {
struct usb_device *dev; /* the corresponding
usb device */
struct usb_interface *control; /* control interface */
@@ -117,7 +122,7 @@ struct acm {
unsigned int throttled:1; /* actually throttled */
unsigned int throttle_req:1;/* throttle requested */
u8 bInterval;
- struct acm_wb *delayed_wb; /* write queued for a
device about to be woken */
+ struct list_head delayed_wb_list; /* delayed wb list */
};
#define CDC_DATA_INTERFACE_TYPE0x0a
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: cdc-acm: put delayed_wb to list
Hi, Greg, I am sorry for the inconvenience. I will the patch format with the title v2. Thanks for the review. On Mon, 2013-10-14 at 11:10 -0700, Greg KH wrote: > On Tue, Oct 08, 2013 at 05:08:48PM +0800, Xiao Jin wrote: > > > > From: xiao jin > > > > If acm_write_start during acm suspend, write acm_wb is backuped > > to delayed_wb. When acm resume, the delayed_wb will be started. > > This mechanism can only record one write during acm suspend. More > > acm write will be abandoned. > > > > This patch is to use list_head to record the write acm_wb during acm > > suspend. It can ensure no acm write abandoned. > > > This patch is line-wrapped, making it impossible to apply. Please check > your email client and fix up to not do this in the future and resend > this patch. > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: cdc-acm: put delayed_wb to list
Hi, Greg, I am sorry for the inconvenience. I will the patch format with the title v2. Thanks for the review. On Mon, 2013-10-14 at 11:10 -0700, Greg KH wrote: On Tue, Oct 08, 2013 at 05:08:48PM +0800, Xiao Jin wrote: From: xiao jin jin.x...@intel.com If acm_write_start during acm suspend, write acm_wb is backuped to delayed_wb. When acm resume, the delayed_wb will be started. This mechanism can only record one write during acm suspend. More acm write will be abandoned. This patch is to use list_head to record the write acm_wb during acm suspend. It can ensure no acm write abandoned. This patch is line-wrapped, making it impossible to apply. Please check your email client and fix up to not do this in the future and resend this patch. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] cdc-acm: put delayed_wb to list
If acm_write_start during acm suspend, write acm_wb is backuped
to delayed_wb. When acm resume, the delayed_wb will be started.
This mechanism can only record one write during acm suspend. More
acm write will be abandoned.
This patch is to use list_head to record the write acm_wb during acm
suspend. It can ensure no acm write abandoned.
Signed-off-by: xiao jin jin.x...@intel.com
---
drivers/usb/class/cdc-acm.c | 30 --
drivers/usb/class/cdc-acm.h |7 ++-
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 9f49bfe..be679be 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -607,6 +607,7 @@ static int acm_tty_write(struct tty_struct *tty,
unsigned long flags;
int wbn;
struct acm_wb *wb;
+ struct delayed_wb *d_wb;
if (!count)
return 0;
@@ -634,12 +635,17 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm-control);
if (acm-susp_count) {
- if (!acm-delayed_wb)
- acm-delayed_wb = wb;
- else
+ d_wb = kmalloc(sizeof(struct delayed_wb), GFP_ATOMIC);
+ if (d_wb == NULL) {
usb_autopm_put_interface_async(acm-control);
- spin_unlock_irqrestore(acm-write_lock, flags);
- return count; /* A white lie */
+ spin_unlock_irqrestore(acm-write_lock, flags);
+ return -ENOMEM;
+ } else {
+ d_wb-wb = wb;
+ list_add_tail(d_wb-list, acm-delayed_wb_list);
+ spin_unlock_irqrestore(acm-write_lock, flags);
+ return count; /* A white lie */
+ }
}
usb_mark_last_busy(acm-dev);
@@ -1255,6 +1261,7 @@ made_compressed_probe:
snd-urb-transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
snd-instance = acm;
}
+ INIT_LIST_HEAD(acm-delayed_wb_list);
usb_set_intfdata(intf, acm);
@@ -1449,6 +1456,7 @@ static int acm_resume(struct usb_interface *intf)
{
struct acm *acm = usb_get_intfdata(intf);
struct acm_wb *wb;
+ struct delayed_wb *d_wb, *nd_wb;
int rv = 0;
int cnt;
@@ -1464,14 +1472,16 @@ static int acm_resume(struct usb_interface *intf)
rv = usb_submit_urb(acm-ctrlurb, GFP_NOIO);
spin_lock_irq(acm-write_lock);
- if (acm-delayed_wb) {
- wb = acm-delayed_wb;
- acm-delayed_wb = NULL;
+ list_for_each_entry_safe(d_wb, nd_wb,
+ acm-delayed_wb_list, list) {
+ wb = d_wb-wb;
+ list_del(d_wb-list);
+ kfree(d_wb);
spin_unlock_irq(acm-write_lock);
acm_start_wb(acm, wb);
- } else {
- spin_unlock_irq(acm-write_lock);
+ spin_lock_irq(acm-write_lock);
}
+ spin_unlock_irq(acm-write_lock);
/*
* delayed error checking because we must
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index 0f76e4a..5eed93f 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -79,6 +79,11 @@ struct acm_rb {
struct acm *instance;
};
+struct delayed_wb {
+ struct list_headlist;
+ struct acm_wb *wb;
+}
+
struct acm {
struct usb_device *dev; /* the corresponding
usb device */
struct usb_interface *control; /* control interface */
@@ -117,7 +122,7 @@ struct acm {
unsigned int throttled:1; /* actually throttled */
unsigned int throttle_req:1;/* throttle requested */
u8 bInterval;
- struct acm_wb *delayed_wb; /* write queued for a
device about to be woken */
+ struct list_head delayed_wb_list; /* delayed wb list */
};
#define CDC_DATA_INTERFACE_TYPE0x0a
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xhci: Ensure a command structure points to the correct trb on the command ring
Sarah,
As you said, I make a mistake and send wrong patch. I am sorry for it.
On Fri, 2013-10-11 at 10:28 -0700, Sarah Sharp wrote:
> On Fri, Oct 11, 2013 at 10:25:23AM -0700, Sarah Sharp wrote:
> > Hi Xiao,
> >
> > I think you did something odd when you tried to send me the latest
> > revision of your patch "xhci: correct the usage of
> > USB_CTRL_SET_TIMEOUT". You sent this patch instead. On the plus side,
> > it looks like git-send-email works.
> >
> > Can you try again?
>
> Ah, nevermind, I saw the v2 patch you sent later.
>
> Sarah
>
> > On Fri, Oct 11, 2013 at 08:47:54AM +0800, xiao jin wrote:
> > > From: Mathias Nyman
> > >
> > > If a command on the command ring needs to be cancelled before it is
> > > handled
> > > it can be turned to a no-op operation when the ring is stopped.
> > > We want to store the command ring enqueue pointer in the command structure
> > > when the command in enqueued for the cancellation case.
> > >
> > > Some commands used to store the command ring dequeue pointers instead of
> > > enqueue
> > > (these often worked because enqueue happends to equal dequeue quite often)
> > >
> > > Other commands correctly used the enqueue pointer but did not check if it
> > > pointed
> > > to a valid trb or a link trb, this caused for example stop endpoint
> > > command to timeout in
> > > xhci_stop_device() in about 2% of suspend/resume cases.
> > >
> > > This should also solve some weird behavior happening in command
> > > cancellation cases.
> > >
> > > This patch is based on a patch submitted by Sarah Sharp to linux-usb, but
> > > then forgotten:
> > > http://marc.info/?l=linux-usb=136269803207465=2
> > >
> > > This patch should be backported to kernels as old as 3.7, that contain
> > > the commit b92cc66c047ff7cf587b318fe377061a353c120f "xHCI: add aborting
> > > command ring function"
> > >
> > > Signed-off-by: Mathias Nyman
> > > Signed-off-by: Sarah Sharp
> > > Cc: sta...@vger.kernel.org
> > > ---
> > > drivers/usb/host/xhci-hub.c |2 +-
> > > drivers/usb/host/xhci-ring.c | 10 ++
> > > drivers/usb/host/xhci.c | 25 +
> > > drivers/usb/host/xhci.h |1 +
> > > 4 files changed, 17 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> > > index fae697e..ccf0a06 100644
> > > --- a/drivers/usb/host/xhci-hub.c
> > > +++ b/drivers/usb/host/xhci-hub.c
> > > @@ -287,7 +287,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci,
> > > int slot_id, int suspend)
> > > if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue)
> > > xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
> > > }
> > > - cmd->command_trb = xhci->cmd_ring->enqueue;
> > > + cmd->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
> > > list_add_tail(>cmd_list, _dev->cmd_list);
> > > xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
> > > xhci_ring_cmd_db(xhci);
> > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > > index aaa2906..9ac9672 100644
> > > --- a/drivers/usb/host/xhci-ring.c
> > > +++ b/drivers/usb/host/xhci-ring.c
> > > @@ -123,6 +123,16 @@ static int enqueue_is_link_trb(struct xhci_ring
> > > *ring)
> > > return TRB_TYPE_LINK_LE32(link->control);
> > > }
> > >
> > > +union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring)
> > > +{
> > > + /* Enqueue pointer can be left pointing to the link TRB,
> > > + * we must handle that
> > > + */
> > > + if (TRB_TYPE_LINK_LE32(ring->enqueue->link.control))
> > > + return ring->enq_seg->next->trbs;
> > > + return ring->enqueue;
> > > +}
> > > +
> > > /* Updates trb to point to the next TRB in the ring, and updates seg if
> > > the next
> > > * TRB is in a new segment. This does not skip over link TRBs, and it
> > > does not
> > > * effect the ring dequeue or enqueue pointers.
> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > index 49b6edb..1e36dbb 100644
> > > --- a/drivers/usb/host/xhci.c
> > > +++ b/drivers/usb/host/xhci.c
>
Re: [PATCH] xhci: Ensure a command structure points to the correct trb on the command ring
Sarah,
As you said, I make a mistake and send wrong patch. I am sorry for it.
On Fri, 2013-10-11 at 10:28 -0700, Sarah Sharp wrote:
On Fri, Oct 11, 2013 at 10:25:23AM -0700, Sarah Sharp wrote:
Hi Xiao,
I think you did something odd when you tried to send me the latest
revision of your patch xhci: correct the usage of
USB_CTRL_SET_TIMEOUT. You sent this patch instead. On the plus side,
it looks like git-send-email works.
Can you try again?
Ah, nevermind, I saw the v2 patch you sent later.
Sarah
On Fri, Oct 11, 2013 at 08:47:54AM +0800, xiao jin wrote:
From: Mathias Nyman mathias.ny...@linux.intel.com
If a command on the command ring needs to be cancelled before it is
handled
it can be turned to a no-op operation when the ring is stopped.
We want to store the command ring enqueue pointer in the command structure
when the command in enqueued for the cancellation case.
Some commands used to store the command ring dequeue pointers instead of
enqueue
(these often worked because enqueue happends to equal dequeue quite often)
Other commands correctly used the enqueue pointer but did not check if it
pointed
to a valid trb or a link trb, this caused for example stop endpoint
command to timeout in
xhci_stop_device() in about 2% of suspend/resume cases.
This should also solve some weird behavior happening in command
cancellation cases.
This patch is based on a patch submitted by Sarah Sharp to linux-usb, but
then forgotten:
http://marc.info/?l=linux-usbm=136269803207465w=2
This patch should be backported to kernels as old as 3.7, that contain
the commit b92cc66c047ff7cf587b318fe377061a353c120f xHCI: add aborting
command ring function
Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com
Signed-off-by: Sarah Sharp sarah.a.sh...@linux.intel.com
Cc: sta...@vger.kernel.org
---
drivers/usb/host/xhci-hub.c |2 +-
drivers/usb/host/xhci-ring.c | 10 ++
drivers/usb/host/xhci.c | 25 +
drivers/usb/host/xhci.h |1 +
4 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index fae697e..ccf0a06 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -287,7 +287,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci,
int slot_id, int suspend)
if (virt_dev-eps[i].ring virt_dev-eps[i].ring-dequeue)
xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
}
- cmd-command_trb = xhci-cmd_ring-enqueue;
+ cmd-command_trb = xhci_find_next_enqueue(xhci-cmd_ring);
list_add_tail(cmd-cmd_list, virt_dev-cmd_list);
xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
xhci_ring_cmd_db(xhci);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index aaa2906..9ac9672 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -123,6 +123,16 @@ static int enqueue_is_link_trb(struct xhci_ring
*ring)
return TRB_TYPE_LINK_LE32(link-control);
}
+union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring)
+{
+ /* Enqueue pointer can be left pointing to the link TRB,
+ * we must handle that
+ */
+ if (TRB_TYPE_LINK_LE32(ring-enqueue-link.control))
+ return ring-enq_seg-next-trbs;
+ return ring-enqueue;
+}
+
/* Updates trb to point to the next TRB in the ring, and updates seg if
the next
* TRB is in a new segment. This does not skip over link TRBs, and it
does not
* effect the ring dequeue or enqueue pointers.
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 49b6edb..1e36dbb 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2598,15 +2598,7 @@ static int xhci_configure_endpoint(struct xhci_hcd
*xhci,
if (command) {
cmd_completion = command-completion;
cmd_status = command-status;
- command-command_trb = xhci-cmd_ring-enqueue;
-
- /* Enqueue pointer can be left pointing to the link TRB,
- * we must handle that
- */
- if (TRB_TYPE_LINK_LE32(command-command_trb-link.control))
- command-command_trb =
- xhci-cmd_ring-enq_seg-next-trbs;
-
+ command-command_trb = xhci_find_next_enqueue(xhci-cmd_ring);
list_add_tail(command-cmd_list, virt_dev-cmd_list);
} else {
cmd_completion = virt_dev-cmd_completion;
@@ -2614,7 +2606,7 @@ static int xhci_configure_endpoint(struct xhci_hcd
*xhci,
}
init_completion(cmd_completion);
- cmd_trb = xhci-cmd_ring-dequeue;
+ cmd_trb = xhci_find_next_enqueue(xhci-cmd_ring);
if (!ctx_change)
ret = xhci_queue_configure_endpoint(xhci
[PATCH v2] xhci: correct the usage of USB_CTRL_SET_TIMEOUT
The usage of USB_CTRL_SET_TIMEOUT in xhci is incorrect.
The definition of USB_CTRL_SET_TIMEOUT is 5000ms. The
input timeout to wait_for_completion_interruptible_timeout
is jiffies. That makes the timeout be longer than what
we want, such as 50s in some platform.
The patch is to use XHCI_CMD_DEFAULT_TIMEOUT instead of
USB_CTRL_SET_TIMEOUT as command completion event timeout.
Signed-off-by: xiao jin
---
drivers/usb/host/xhci-hub.c |2 +-
drivers/usb/host/xhci.c |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 1d35459..8dbaa81 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -295,7 +295,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int
slot_id, int suspend)
/* Wait for last stop endpoint command to finish */
timeleft = wait_for_completion_interruptible_timeout(
cmd->completion,
- USB_CTRL_SET_TIMEOUT);
+ XHCI_CMD_DEFAULT_TIMEOUT);
if (timeleft <= 0) {
xhci_warn(xhci, "%s while waiting for stop endpoint command\n",
timeleft == 0 ? "Timeout" : "Signal");
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9478caa..5dc6903 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3486,7 +3486,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd,
struct usb_device *udev)
/* Wait for the Reset Device command to finish */
timeleft = wait_for_completion_interruptible_timeout(
reset_device_cmd->completion,
- USB_CTRL_SET_TIMEOUT);
+ XHCI_CMD_DEFAULT_TIMEOUT);
if (timeleft <= 0) {
xhci_warn(xhci, "%s while waiting for reset device command\n",
timeleft == 0 ? "Timeout" : "Signal");
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] xhci: Ensure a command structure points to the correct trb on the command ring
From: Mathias Nyman
If a command on the command ring needs to be cancelled before it is handled
it can be turned to a no-op operation when the ring is stopped.
We want to store the command ring enqueue pointer in the command structure
when the command in enqueued for the cancellation case.
Some commands used to store the command ring dequeue pointers instead of enqueue
(these often worked because enqueue happends to equal dequeue quite often)
Other commands correctly used the enqueue pointer but did not check if it
pointed
to a valid trb or a link trb, this caused for example stop endpoint command to
timeout in
xhci_stop_device() in about 2% of suspend/resume cases.
This should also solve some weird behavior happening in command cancellation
cases.
This patch is based on a patch submitted by Sarah Sharp to linux-usb, but
then forgotten:
http://marc.info/?l=linux-usb=136269803207465=2
This patch should be backported to kernels as old as 3.7, that contain
the commit b92cc66c047ff7cf587b318fe377061a353c120f "xHCI: add aborting
command ring function"
Signed-off-by: Mathias Nyman
Signed-off-by: Sarah Sharp
Cc: sta...@vger.kernel.org
---
drivers/usb/host/xhci-hub.c |2 +-
drivers/usb/host/xhci-ring.c | 10 ++
drivers/usb/host/xhci.c | 25 +
drivers/usb/host/xhci.h |1 +
4 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index fae697e..ccf0a06 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -287,7 +287,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int
slot_id, int suspend)
if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue)
xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
}
- cmd->command_trb = xhci->cmd_ring->enqueue;
+ cmd->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
list_add_tail(>cmd_list, _dev->cmd_list);
xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
xhci_ring_cmd_db(xhci);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index aaa2906..9ac9672 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -123,6 +123,16 @@ static int enqueue_is_link_trb(struct xhci_ring *ring)
return TRB_TYPE_LINK_LE32(link->control);
}
+union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring)
+{
+ /* Enqueue pointer can be left pointing to the link TRB,
+* we must handle that
+*/
+ if (TRB_TYPE_LINK_LE32(ring->enqueue->link.control))
+ return ring->enq_seg->next->trbs;
+ return ring->enqueue;
+}
+
/* Updates trb to point to the next TRB in the ring, and updates seg if the
next
* TRB is in a new segment. This does not skip over link TRBs, and it does not
* effect the ring dequeue or enqueue pointers.
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 49b6edb..1e36dbb 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2598,15 +2598,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
if (command) {
cmd_completion = command->completion;
cmd_status = >status;
- command->command_trb = xhci->cmd_ring->enqueue;
-
- /* Enqueue pointer can be left pointing to the link TRB,
-* we must handle that
-*/
- if (TRB_TYPE_LINK_LE32(command->command_trb->link.control))
- command->command_trb =
- xhci->cmd_ring->enq_seg->next->trbs;
-
+ command->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
list_add_tail(>cmd_list, _dev->cmd_list);
} else {
cmd_completion = _dev->cmd_completion;
@@ -2614,7 +2606,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
}
init_completion(cmd_completion);
- cmd_trb = xhci->cmd_ring->dequeue;
+ cmd_trb = xhci_find_next_enqueue(xhci->cmd_ring);
if (!ctx_change)
ret = xhci_queue_configure_endpoint(xhci, in_ctx->dma,
udev->slot_id, must_succeed);
@@ -3439,14 +3431,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd,
struct usb_device *udev)
/* Attempt to submit the Reset Device command to the command ring */
spin_lock_irqsave(>lock, flags);
- reset_device_cmd->command_trb = xhci->cmd_ring->enqueue;
-
- /* Enqueue pointer can be left pointing to the link TRB,
-* we must handle that
-*/
- if (TRB_TYPE_LINK_LE32(reset_device_cmd->command_trb->link.control))
- reset_device_cmd->command_trb =
- xhci->cmd_ring->enq_seg->next->trbs;
+ reset_device_cmd->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
list_add_tail(_device_cmd->cmd_list,
[PATCH] xhci: Ensure a command structure points to the correct trb on the command ring
From: Mathias Nyman mathias.ny...@linux.intel.com
If a command on the command ring needs to be cancelled before it is handled
it can be turned to a no-op operation when the ring is stopped.
We want to store the command ring enqueue pointer in the command structure
when the command in enqueued for the cancellation case.
Some commands used to store the command ring dequeue pointers instead of enqueue
(these often worked because enqueue happends to equal dequeue quite often)
Other commands correctly used the enqueue pointer but did not check if it
pointed
to a valid trb or a link trb, this caused for example stop endpoint command to
timeout in
xhci_stop_device() in about 2% of suspend/resume cases.
This should also solve some weird behavior happening in command cancellation
cases.
This patch is based on a patch submitted by Sarah Sharp to linux-usb, but
then forgotten:
http://marc.info/?l=linux-usbm=136269803207465w=2
This patch should be backported to kernels as old as 3.7, that contain
the commit b92cc66c047ff7cf587b318fe377061a353c120f xHCI: add aborting
command ring function
Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com
Signed-off-by: Sarah Sharp sarah.a.sh...@linux.intel.com
Cc: sta...@vger.kernel.org
---
drivers/usb/host/xhci-hub.c |2 +-
drivers/usb/host/xhci-ring.c | 10 ++
drivers/usb/host/xhci.c | 25 +
drivers/usb/host/xhci.h |1 +
4 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index fae697e..ccf0a06 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -287,7 +287,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int
slot_id, int suspend)
if (virt_dev-eps[i].ring virt_dev-eps[i].ring-dequeue)
xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
}
- cmd-command_trb = xhci-cmd_ring-enqueue;
+ cmd-command_trb = xhci_find_next_enqueue(xhci-cmd_ring);
list_add_tail(cmd-cmd_list, virt_dev-cmd_list);
xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
xhci_ring_cmd_db(xhci);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index aaa2906..9ac9672 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -123,6 +123,16 @@ static int enqueue_is_link_trb(struct xhci_ring *ring)
return TRB_TYPE_LINK_LE32(link-control);
}
+union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring)
+{
+ /* Enqueue pointer can be left pointing to the link TRB,
+* we must handle that
+*/
+ if (TRB_TYPE_LINK_LE32(ring-enqueue-link.control))
+ return ring-enq_seg-next-trbs;
+ return ring-enqueue;
+}
+
/* Updates trb to point to the next TRB in the ring, and updates seg if the
next
* TRB is in a new segment. This does not skip over link TRBs, and it does not
* effect the ring dequeue or enqueue pointers.
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 49b6edb..1e36dbb 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2598,15 +2598,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
if (command) {
cmd_completion = command-completion;
cmd_status = command-status;
- command-command_trb = xhci-cmd_ring-enqueue;
-
- /* Enqueue pointer can be left pointing to the link TRB,
-* we must handle that
-*/
- if (TRB_TYPE_LINK_LE32(command-command_trb-link.control))
- command-command_trb =
- xhci-cmd_ring-enq_seg-next-trbs;
-
+ command-command_trb = xhci_find_next_enqueue(xhci-cmd_ring);
list_add_tail(command-cmd_list, virt_dev-cmd_list);
} else {
cmd_completion = virt_dev-cmd_completion;
@@ -2614,7 +2606,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
}
init_completion(cmd_completion);
- cmd_trb = xhci-cmd_ring-dequeue;
+ cmd_trb = xhci_find_next_enqueue(xhci-cmd_ring);
if (!ctx_change)
ret = xhci_queue_configure_endpoint(xhci, in_ctx-dma,
udev-slot_id, must_succeed);
@@ -3439,14 +3431,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd,
struct usb_device *udev)
/* Attempt to submit the Reset Device command to the command ring */
spin_lock_irqsave(xhci-lock, flags);
- reset_device_cmd-command_trb = xhci-cmd_ring-enqueue;
-
- /* Enqueue pointer can be left pointing to the link TRB,
-* we must handle that
-*/
- if (TRB_TYPE_LINK_LE32(reset_device_cmd-command_trb-link.control))
- reset_device_cmd-command_trb =
- xhci-cmd_ring-enq_seg-next-trbs;
+ reset_device_cmd-command_trb =
[PATCH v2] xhci: correct the usage of USB_CTRL_SET_TIMEOUT
The usage of USB_CTRL_SET_TIMEOUT in xhci is incorrect.
The definition of USB_CTRL_SET_TIMEOUT is 5000ms. The
input timeout to wait_for_completion_interruptible_timeout
is jiffies. That makes the timeout be longer than what
we want, such as 50s in some platform.
The patch is to use XHCI_CMD_DEFAULT_TIMEOUT instead of
USB_CTRL_SET_TIMEOUT as command completion event timeout.
Signed-off-by: xiao jin jin.x...@intel.com
---
drivers/usb/host/xhci-hub.c |2 +-
drivers/usb/host/xhci.c |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 1d35459..8dbaa81 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -295,7 +295,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int
slot_id, int suspend)
/* Wait for last stop endpoint command to finish */
timeleft = wait_for_completion_interruptible_timeout(
cmd-completion,
- USB_CTRL_SET_TIMEOUT);
+ XHCI_CMD_DEFAULT_TIMEOUT);
if (timeleft = 0) {
xhci_warn(xhci, %s while waiting for stop endpoint command\n,
timeleft == 0 ? Timeout : Signal);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9478caa..5dc6903 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3486,7 +3486,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd,
struct usb_device *udev)
/* Wait for the Reset Device command to finish */
timeleft = wait_for_completion_interruptible_timeout(
reset_device_cmd-completion,
- USB_CTRL_SET_TIMEOUT);
+ XHCI_CMD_DEFAULT_TIMEOUT);
if (timeleft = 0) {
xhci_warn(xhci, %s while waiting for reset device command\n,
timeleft == 0 ? Timeout : Signal);
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xhci-hub.c: handle command_trb that may be link TRB
Sarah,
Thanks for the reminding. The kernel base of the patch is k3.10, I
didn't notice k3.12 commit ec7e43e2d98173483866fe2e4e690143626b659c at
that time. I will backport the patch from k3.12 for use.
As you mentioned in another email, we meet with dpm timeout when xhci
suspend, the root cause are what the patch aimed to solve. FYI.
On Wed, 2013-10-09 at 13:18 -0700, Sarah Sharp wrote:
> Hi Xiao,
>
> Thanks for taking the time to submit this patch. Comments below.
>
> On Wed, Oct 09, 2013 at 09:42:36AM +0800, Xiao Jin wrote:
> > From: xiao jin
> > Date: Wed, 9 Oct 2013 09:38:45 +0800
> > Subject: [PATCH] xhci-hub.c: handle command_trb that may be link TRB
>
> I won't be able to apply your patch because it has these extra lines in
> the body ^^^. I suspect you used `git format-patch` to produce this,
> and then tried to copy-paste it into your mail client? You can't do
> that, because your mail client will probably word-wrap your patch, and
> possibly turn tabs into spaces. You need to use `git send-email`
> instead to send your patches.
>
> > When xhci stop device, it's possible cmd_ring enqueue point to
> > link TRB after queue the last but one stop endpoint. We must
> > handle the command_trb point to the next segment trb. Otherwise
> > xhci stop devie will timeout because command_trb can't match
> > with cmd_ring dequeue.
> >
> > The patch is to let command_trb point to the next segment trb if
> > cmd_ring enqueue point to link TRB.
> >
> > Signed-off-by: xiao jin
> > ---
> > drivers/usb/host/xhci-hub.c |7 +++
> > 1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> > index 1d35459..4872640 100644
> > --- a/drivers/usb/host/xhci-hub.c
> > +++ b/drivers/usb/host/xhci-hub.c
> > @@ -287,6 +287,13 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int
> > slot_id, int suspend)
> > xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
> > }
> > cmd->command_trb = xhci->cmd_ring->enqueue;
> > + /* Enqueue pointer can be left pointing to the link TRB,
> > +* we must handle that
> > +*/
> > + if (TRB_TYPE_LINK_LE32(cmd->command_trb->link.control))
> > + cmd->command_trb =
> > + xhci->cmd_ring->enq_seg->next->trbs;
> > +
> > list_add_tail(>cmd_list, _dev->cmd_list);
> > xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
> > xhci_ring_cmd_db(xhci);
>
> What kernel version or tree is it against? I ask because there's
> already a fix in the 3.12-rc4 kernel for this:
>
> static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
> {
> ...
> spin_lock_irqsave(>lock, flags);
> for (i = LAST_EP_INDEX; i > 0; i--) {
> if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue)
> xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
> }
> cmd->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
> ...
>
> Where xhci_find_next_enqueue is defined as:
>
> union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring)
> {
> /* Enqueue pointer can be left pointing to the link TRB,
> * we must handle that
> */
> if (TRB_TYPE_LINK_LE32(ring->enqueue->link.control))
> return ring->enq_seg->next->trbs;
> return ring->enqueue;
> }
>
> That was added in commit ec7e43e2d98173483866fe2e4e690143626b659c "xhci:
> Ensure a command structure points to the correct trb on the command
> ring" It's in (or should be in soon) the stable kernels as well.
>
> Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xhci-hub.c: handle command_trb that may be link TRB
Sarah,
Thanks for the reminding. The kernel base of the patch is k3.10, I
didn't notice k3.12 commit ec7e43e2d98173483866fe2e4e690143626b659c at
that time. I will backport the patch from k3.12 for use.
As you mentioned in another email, we meet with dpm timeout when xhci
suspend, the root cause are what the patch aimed to solve. FYI.
On Wed, 2013-10-09 at 13:18 -0700, Sarah Sharp wrote:
Hi Xiao,
Thanks for taking the time to submit this patch. Comments below.
On Wed, Oct 09, 2013 at 09:42:36AM +0800, Xiao Jin wrote:
From: xiao jin jin.x...@intel.com
Date: Wed, 9 Oct 2013 09:38:45 +0800
Subject: [PATCH] xhci-hub.c: handle command_trb that may be link TRB
I won't be able to apply your patch because it has these extra lines in
the body ^^^. I suspect you used `git format-patch` to produce this,
and then tried to copy-paste it into your mail client? You can't do
that, because your mail client will probably word-wrap your patch, and
possibly turn tabs into spaces. You need to use `git send-email`
instead to send your patches.
When xhci stop device, it's possible cmd_ring enqueue point to
link TRB after queue the last but one stop endpoint. We must
handle the command_trb point to the next segment trb. Otherwise
xhci stop devie will timeout because command_trb can't match
with cmd_ring dequeue.
The patch is to let command_trb point to the next segment trb if
cmd_ring enqueue point to link TRB.
Signed-off-by: xiao jin jin.x...@intel.com
---
drivers/usb/host/xhci-hub.c |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 1d35459..4872640 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -287,6 +287,13 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int
slot_id, int suspend)
xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
}
cmd-command_trb = xhci-cmd_ring-enqueue;
+ /* Enqueue pointer can be left pointing to the link TRB,
+* we must handle that
+*/
+ if (TRB_TYPE_LINK_LE32(cmd-command_trb-link.control))
+ cmd-command_trb =
+ xhci-cmd_ring-enq_seg-next-trbs;
+
list_add_tail(cmd-cmd_list, virt_dev-cmd_list);
xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
xhci_ring_cmd_db(xhci);
What kernel version or tree is it against? I ask because there's
already a fix in the 3.12-rc4 kernel for this:
static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
{
...
spin_lock_irqsave(xhci-lock, flags);
for (i = LAST_EP_INDEX; i 0; i--) {
if (virt_dev-eps[i].ring virt_dev-eps[i].ring-dequeue)
xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
}
cmd-command_trb = xhci_find_next_enqueue(xhci-cmd_ring);
...
Where xhci_find_next_enqueue is defined as:
union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring)
{
/* Enqueue pointer can be left pointing to the link TRB,
* we must handle that
*/
if (TRB_TYPE_LINK_LE32(ring-enqueue-link.control))
return ring-enq_seg-next-trbs;
return ring-enqueue;
}
That was added in commit ec7e43e2d98173483866fe2e4e690143626b659c xhci:
Ensure a command structure points to the correct trb on the command
ring It's in (or should be in soon) the stable kernels as well.
Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] xhci-hub.c: handle command_trb that may be link TRB
From: xiao jin Date: Wed, 9 Oct 2013 09:38:45 +0800 Subject: [PATCH] xhci-hub.c: handle command_trb that may be link TRB When xhci stop device, it's possible cmd_ring enqueue point to link TRB after queue the last but one stop endpoint. We must handle the command_trb point to the next segment trb. Otherwise xhci stop devie will timeout because command_trb can't match with cmd_ring dequeue. The patch is to let command_trb point to the next segment trb if cmd_ring enqueue point to link TRB. Signed-off-by: xiao jin --- drivers/usb/host/xhci-hub.c |7 +++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 1d35459..4872640 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -287,6 +287,13 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) xhci_queue_stop_endpoint(xhci, slot_id, i, suspend); } cmd->command_trb = xhci->cmd_ring->enqueue; + /* Enqueue pointer can be left pointing to the link TRB, +* we must handle that +*/ + if (TRB_TYPE_LINK_LE32(cmd->command_trb->link.control)) + cmd->command_trb = + xhci->cmd_ring->enq_seg->next->trbs; + list_add_tail(>cmd_list, _dev->cmd_list); xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend); xhci_ring_cmd_db(xhci); -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] xhci: correct the usage of USB_CTRL_SET_TIMEOUT
From: xiao jin
Date: Wed, 9 Oct 2013 09:09:46 +0800
Subject: [PATCH] xhci: correct the usage of USB_CTRL_SET_TIMEOUT
The usage of USB_CTRL_SET_TIMEOUT is incorrect. The
definition of USB_CTRL_SET_TIMEOUT is 5000ms. The
input timeout to wait_for_completion_interruptible_timeout
is jiffies. That makes the timeout be longer than what
we want, such as 50s in some platform.
The patch is to convert USB_CTRL_SET_TIMEOUT to jiffies as
command completion event timeout.
Signed-off-by: xiao jin
---
drivers/usb/host/xhci-hub.c |2 +-
drivers/usb/host/xhci.c |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 1d35459..78cf294 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -295,7 +295,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int
slot_id, int suspend)
/* Wait for last stop endpoint command to finish */
timeleft = wait_for_completion_interruptible_timeout(
cmd->completion,
- USB_CTRL_SET_TIMEOUT);
+ msecs_to_jiffies(USB_CTRL_SET_TIMEOUT));
if (timeleft <= 0) {
xhci_warn(xhci, "%s while waiting for stop endpoint command\n",
timeleft == 0 ? "Timeout" : "Signal");
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9478caa..f9ebc72 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3486,7 +3486,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd,
struct usb_device *udev)
/* Wait for the Reset Device command to finish */
timeleft = wait_for_completion_interruptible_timeout(
reset_device_cmd->completion,
- USB_CTRL_SET_TIMEOUT);
+ msecs_to_jiffies(USB_CTRL_SET_TIMEOUT));
if (timeleft <= 0) {
xhci_warn(xhci, "%s while waiting for reset device command\n",
timeleft == 0 ? "Timeout" : "Signal");
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] cdc-acm: put delayed_wb to list
From: xiao jin
Date: Tue, 8 Oct 2013 16:57:29 +0800
Subject: [PATCH] cdc-acm: put delayed_wb to list
If acm_write_start during acm suspend, write acm_wb is backuped
to delayed_wb. When acm resume, the delayed_wb will be started.
This mechanism can only record one write during acm suspend. More
acm write will be abandoned.
This patch is to use list_head to record the write acm_wb during acm
suspend. It can ensure no acm write abandoned.
Signed-off-by: xiao jin
---
drivers/usb/class/cdc-acm.c | 30 --
drivers/usb/class/cdc-acm.h |7 ++-
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 9f49bfe..be679be 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -607,6 +607,7 @@ static int acm_tty_write(struct tty_struct *tty,
unsigned long flags;
int wbn;
struct acm_wb *wb;
+ struct delayed_wb *d_wb;
if (!count)
return 0;
@@ -634,12 +635,17 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm->control);
if (acm->susp_count) {
- if (!acm->delayed_wb)
- acm->delayed_wb = wb;
- else
+ d_wb = kmalloc(sizeof(struct delayed_wb), GFP_ATOMIC);
+ if (d_wb == NULL) {
usb_autopm_put_interface_async(acm->control);
- spin_unlock_irqrestore(>write_lock, flags);
- return count; /* A white lie */
+ spin_unlock_irqrestore(>write_lock, flags);
+ return -ENOMEM;
+ } else {
+ d_wb->wb = wb;
+ list_add_tail(_wb->list, >delayed_wb_list);
+ spin_unlock_irqrestore(>write_lock, flags);
+ return count; /* A white lie */
+ }
}
usb_mark_last_busy(acm->dev);
@@ -1255,6 +1261,7 @@ made_compressed_probe:
snd->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
snd->instance = acm;
}
+ INIT_LIST_HEAD(>delayed_wb_list);
usb_set_intfdata(intf, acm);
@@ -1449,6 +1456,7 @@ static int acm_resume(struct usb_interface *intf)
{
struct acm *acm = usb_get_intfdata(intf);
struct acm_wb *wb;
+ struct delayed_wb *d_wb, *nd_wb;
int rv = 0;
int cnt;
@@ -1464,14 +1472,16 @@ static int acm_resume(struct usb_interface
*intf)
rv = usb_submit_urb(acm->ctrlurb, GFP_NOIO);
spin_lock_irq(>write_lock);
- if (acm->delayed_wb) {
- wb = acm->delayed_wb;
- acm->delayed_wb = NULL;
+ list_for_each_entry_safe(d_wb, nd_wb,
+ >delayed_wb_list, list) {
+ wb = d_wb->wb;
+ list_del(_wb->list);
+ kfree(d_wb);
spin_unlock_irq(>write_lock);
acm_start_wb(acm, wb);
- } else {
- spin_unlock_irq(>write_lock);
+ spin_lock_irq(>write_lock);
}
+ spin_unlock_irq(>write_lock);
/*
* delayed error checking because we must
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index 0f76e4a..5eed93f 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -79,6 +79,11 @@ struct acm_rb {
struct acm *instance;
};
+struct delayed_wb {
+ struct list_headlist;
+ struct acm_wb *wb;
+}
+
struct acm {
struct usb_device *dev; /* the corresponding
usb device */
struct usb_interface *control; /* control interface */
@@ -117,7 +122,7 @@ struct acm {
unsigned int throttled:1; /* actually throttled */
unsigned int throttle_req:1;/* throttle requested */
u8 bInterval;
- struct acm_wb *delayed_wb; /* write queued for a
device about to be
woken */
+ struct list_head delayed_wb_list; /* delayed wb list */
};
#define CDC_DATA_INTERFACE_TYPE0x0a
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] cdc-acm: put delayed_wb to list
From: xiao jin jin.x...@intel.com
Date: Tue, 8 Oct 2013 16:57:29 +0800
Subject: [PATCH] cdc-acm: put delayed_wb to list
If acm_write_start during acm suspend, write acm_wb is backuped
to delayed_wb. When acm resume, the delayed_wb will be started.
This mechanism can only record one write during acm suspend. More
acm write will be abandoned.
This patch is to use list_head to record the write acm_wb during acm
suspend. It can ensure no acm write abandoned.
Signed-off-by: xiao jin jin.x...@intel.com
---
drivers/usb/class/cdc-acm.c | 30 --
drivers/usb/class/cdc-acm.h |7 ++-
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 9f49bfe..be679be 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -607,6 +607,7 @@ static int acm_tty_write(struct tty_struct *tty,
unsigned long flags;
int wbn;
struct acm_wb *wb;
+ struct delayed_wb *d_wb;
if (!count)
return 0;
@@ -634,12 +635,17 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm-control);
if (acm-susp_count) {
- if (!acm-delayed_wb)
- acm-delayed_wb = wb;
- else
+ d_wb = kmalloc(sizeof(struct delayed_wb), GFP_ATOMIC);
+ if (d_wb == NULL) {
usb_autopm_put_interface_async(acm-control);
- spin_unlock_irqrestore(acm-write_lock, flags);
- return count; /* A white lie */
+ spin_unlock_irqrestore(acm-write_lock, flags);
+ return -ENOMEM;
+ } else {
+ d_wb-wb = wb;
+ list_add_tail(d_wb-list, acm-delayed_wb_list);
+ spin_unlock_irqrestore(acm-write_lock, flags);
+ return count; /* A white lie */
+ }
}
usb_mark_last_busy(acm-dev);
@@ -1255,6 +1261,7 @@ made_compressed_probe:
snd-urb-transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
snd-instance = acm;
}
+ INIT_LIST_HEAD(acm-delayed_wb_list);
usb_set_intfdata(intf, acm);
@@ -1449,6 +1456,7 @@ static int acm_resume(struct usb_interface *intf)
{
struct acm *acm = usb_get_intfdata(intf);
struct acm_wb *wb;
+ struct delayed_wb *d_wb, *nd_wb;
int rv = 0;
int cnt;
@@ -1464,14 +1472,16 @@ static int acm_resume(struct usb_interface
*intf)
rv = usb_submit_urb(acm-ctrlurb, GFP_NOIO);
spin_lock_irq(acm-write_lock);
- if (acm-delayed_wb) {
- wb = acm-delayed_wb;
- acm-delayed_wb = NULL;
+ list_for_each_entry_safe(d_wb, nd_wb,
+ acm-delayed_wb_list, list) {
+ wb = d_wb-wb;
+ list_del(d_wb-list);
+ kfree(d_wb);
spin_unlock_irq(acm-write_lock);
acm_start_wb(acm, wb);
- } else {
- spin_unlock_irq(acm-write_lock);
+ spin_lock_irq(acm-write_lock);
}
+ spin_unlock_irq(acm-write_lock);
/*
* delayed error checking because we must
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index 0f76e4a..5eed93f 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -79,6 +79,11 @@ struct acm_rb {
struct acm *instance;
};
+struct delayed_wb {
+ struct list_headlist;
+ struct acm_wb *wb;
+}
+
struct acm {
struct usb_device *dev; /* the corresponding
usb device */
struct usb_interface *control; /* control interface */
@@ -117,7 +122,7 @@ struct acm {
unsigned int throttled:1; /* actually throttled */
unsigned int throttle_req:1;/* throttle requested */
u8 bInterval;
- struct acm_wb *delayed_wb; /* write queued for a
device about to be
woken */
+ struct list_head delayed_wb_list; /* delayed wb list */
};
#define CDC_DATA_INTERFACE_TYPE0x0a
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] xhci: correct the usage of USB_CTRL_SET_TIMEOUT
From: xiao jin jin.x...@intel.com
Date: Wed, 9 Oct 2013 09:09:46 +0800
Subject: [PATCH] xhci: correct the usage of USB_CTRL_SET_TIMEOUT
The usage of USB_CTRL_SET_TIMEOUT is incorrect. The
definition of USB_CTRL_SET_TIMEOUT is 5000ms. The
input timeout to wait_for_completion_interruptible_timeout
is jiffies. That makes the timeout be longer than what
we want, such as 50s in some platform.
The patch is to convert USB_CTRL_SET_TIMEOUT to jiffies as
command completion event timeout.
Signed-off-by: xiao jin jin.x...@intel.com
---
drivers/usb/host/xhci-hub.c |2 +-
drivers/usb/host/xhci.c |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 1d35459..78cf294 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -295,7 +295,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int
slot_id, int suspend)
/* Wait for last stop endpoint command to finish */
timeleft = wait_for_completion_interruptible_timeout(
cmd-completion,
- USB_CTRL_SET_TIMEOUT);
+ msecs_to_jiffies(USB_CTRL_SET_TIMEOUT));
if (timeleft = 0) {
xhci_warn(xhci, %s while waiting for stop endpoint command\n,
timeleft == 0 ? Timeout : Signal);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9478caa..f9ebc72 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3486,7 +3486,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd,
struct usb_device *udev)
/* Wait for the Reset Device command to finish */
timeleft = wait_for_completion_interruptible_timeout(
reset_device_cmd-completion,
- USB_CTRL_SET_TIMEOUT);
+ msecs_to_jiffies(USB_CTRL_SET_TIMEOUT));
if (timeleft = 0) {
xhci_warn(xhci, %s while waiting for reset device command\n,
timeleft == 0 ? Timeout : Signal);
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] xhci-hub.c: handle command_trb that may be link TRB
From: xiao jin jin.x...@intel.com Date: Wed, 9 Oct 2013 09:38:45 +0800 Subject: [PATCH] xhci-hub.c: handle command_trb that may be link TRB When xhci stop device, it's possible cmd_ring enqueue point to link TRB after queue the last but one stop endpoint. We must handle the command_trb point to the next segment trb. Otherwise xhci stop devie will timeout because command_trb can't match with cmd_ring dequeue. The patch is to let command_trb point to the next segment trb if cmd_ring enqueue point to link TRB. Signed-off-by: xiao jin jin.x...@intel.com --- drivers/usb/host/xhci-hub.c |7 +++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 1d35459..4872640 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -287,6 +287,13 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) xhci_queue_stop_endpoint(xhci, slot_id, i, suspend); } cmd-command_trb = xhci-cmd_ring-enqueue; + /* Enqueue pointer can be left pointing to the link TRB, +* we must handle that +*/ + if (TRB_TYPE_LINK_LE32(cmd-command_trb-link.control)) + cmd-command_trb = + xhci-cmd_ring-enq_seg-next-trbs; + list_add_tail(cmd-cmd_list, virt_dev-cmd_list); xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend); xhci_ring_cmd_db(xhci); -- 1.7.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] n_gsm.c: add tx_lock in gsm_send
Alan, Thanks. But the comment makes me confused. As we see, gsm->output is called by gsm_data_kick too, and it's in the tx_lock... Best regards, Jin Xiao > From: xiaojin > Date: Wed, 19 Dec 2012 11:53:43 +0800 > Subject: [PATCH] n_gsm.c: add tx_lock in gsm_send > > All the call to gsm->output should be in the tx_lock, that could avoid > potential race from MUX level. But we have no tx_lock in gsm_send. > > This patch is to add tx_lock in gsm_send. gsm->output calls the transmit method of the underlying tty driver. We can't do that with interrupts off as some drivers expect to be able to sleep in their output paths. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] n_gsm.c: add tx_lock in gsm_send
Alan, Thanks. But the comment makes me confused. As we see, gsm-output is called by gsm_data_kick too, and it's in the tx_lock... Best regards, Jin Xiao From: xiaojin jin.x...@intel.com Date: Wed, 19 Dec 2012 11:53:43 +0800 Subject: [PATCH] n_gsm.c: add tx_lock in gsm_send All the call to gsm-output should be in the tx_lock, that could avoid potential race from MUX level. But we have no tx_lock in gsm_send. This patch is to add tx_lock in gsm_send. gsm-output calls the transmit method of the underlying tty driver. We can't do that with interrupts off as some drivers expect to be able to sleep in their output paths. Alan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] n_gsm.c: add tx_lock in gsm_send
From: xiaojin
Date: Wed, 19 Dec 2012 11:53:43 +0800
Subject: [PATCH] n_gsm.c: add tx_lock in gsm_send
All the call to gsm->output should be in the tx_lock,
that could avoid potential race from MUX level. But
we have no tx_lock in gsm_send.
This patch is to add tx_lock in gsm_send.
Signed-off-by: xiaojin
---
drivers/tty/n_gsm.c |3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index dcc0430..4572117 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -573,6 +573,7 @@ static void gsm_send(struct gsm_mux *gsm, int addr,
int cr, int control)
int len;
u8 cbuf[10];
u8 ibuf[3];
+ unsigned long flags;
switch (gsm->encoding) {
case 0:
@@ -602,7 +603,9 @@ static void gsm_send(struct gsm_mux *gsm, int addr,
int cr, int control)
WARN_ON(1);
return;
}
+ spin_lock_irqsave(>tx_lock, flags);
gsm->output(gsm, cbuf, len);
+ spin_unlock_irqrestore(>tx_lock, flags);
gsm_print_packet("-->", addr, cr, control, NULL, 0);
}
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] n_gsm.c: add tx_lock in gsm_send
From: xiaojin
Date: Wed, 19 Dec 2012 11:53:43 +0800
Subject: [PATCH] n_gsm.c: add tx_lock in gsm_send
All the call to gsm->output should be in the tx_lock,
that could avoid potential race from MUX level. But
we have no tx_lock in gsm_send.
This patch is to add tx_lock in gsm_send.
Signed-off-by: xiaojin
---
drivers/tty/n_gsm.c |3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index dcc0430..4572117 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -573,6 +573,7 @@ static void gsm_send(struct gsm_mux *gsm, int addr,
int cr, int control)
int len;
u8 cbuf[10];
u8 ibuf[3];
+ unsigned long flags;
switch (gsm->encoding) {
case 0:
@@ -602,7 +603,9 @@ static void gsm_send(struct gsm_mux *gsm, int addr,
int cr, int control)
WARN_ON(1);
return;
}
+ spin_lock_irqsave(>tx_lock, flags);
gsm->output(gsm, cbuf, len);
+ spin_unlock_irqrestore(>tx_lock, flags);
gsm_print_packet("-->", addr, cr, control, NULL, 0);
}
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] n_gsm.c: add tx_lock in gsm_send
From: xiaojin jin.x...@intel.com
Date: Wed, 19 Dec 2012 11:53:43 +0800
Subject: [PATCH] n_gsm.c: add tx_lock in gsm_send
All the call to gsm-output should be in the tx_lock,
that could avoid potential race from MUX level. But
we have no tx_lock in gsm_send.
This patch is to add tx_lock in gsm_send.
Signed-off-by: xiaojin jin.x...@intel.com
---
drivers/tty/n_gsm.c |3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index dcc0430..4572117 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -573,6 +573,7 @@ static void gsm_send(struct gsm_mux *gsm, int addr,
int cr, int control)
int len;
u8 cbuf[10];
u8 ibuf[3];
+ unsigned long flags;
switch (gsm-encoding) {
case 0:
@@ -602,7 +603,9 @@ static void gsm_send(struct gsm_mux *gsm, int addr,
int cr, int control)
WARN_ON(1);
return;
}
+ spin_lock_irqsave(gsm-tx_lock, flags);
gsm-output(gsm, cbuf, len);
+ spin_unlock_irqrestore(gsm-tx_lock, flags);
gsm_print_packet(--, addr, cr, control, NULL, 0);
}
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] n_gsm.c: add tx_lock in gsm_send
From: xiaojin jin.x...@intel.com
Date: Wed, 19 Dec 2012 11:53:43 +0800
Subject: [PATCH] n_gsm.c: add tx_lock in gsm_send
All the call to gsm-output should be in the tx_lock,
that could avoid potential race from MUX level. But
we have no tx_lock in gsm_send.
This patch is to add tx_lock in gsm_send.
Signed-off-by: xiaojin jin.x...@intel.com
---
drivers/tty/n_gsm.c |3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index dcc0430..4572117 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -573,6 +573,7 @@ static void gsm_send(struct gsm_mux *gsm, int addr,
int cr, int control)
int len;
u8 cbuf[10];
u8 ibuf[3];
+ unsigned long flags;
switch (gsm-encoding) {
case 0:
@@ -602,7 +603,9 @@ static void gsm_send(struct gsm_mux *gsm, int addr,
int cr, int control)
WARN_ON(1);
return;
}
+ spin_lock_irqsave(gsm-tx_lock, flags);
gsm-output(gsm, cbuf, len);
+ spin_unlock_irqrestore(gsm-tx_lock, flags);
gsm_print_packet(--, addr, cr, control, NULL, 0);
}
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] spi-dw: delete cur_msg when pump message and transfer has finished
From: xiao jin Date: Tue, 20 Nov 2012 13:45:47 +0800 Subject: [PATCH] spi-dw: delete cur_msg when pump message and transfer has finished We see DPM device timeout happened in spi_max3111 when system suspend. The root cause is related to the time we delete msg from dw_spi queue. Below is the scenario how timeout happened. step1, max3110_main_thread pump the dw_spi msg and transfer it. And the msg is deleted from dw_spi queue right now. step2, runtime suspend happened, there is no msg in the dw_spi queue, it's permitted to enter into D0i3. step3, After resume from D0i3, irq will not come for ever, max3110_main_thread still wait for the completion of last pump message transfer. step4, system suspend, serial_m3110_shutdown call max3110_out and try to lock the mutex, but it has been hold and cannot be released by max3110_main_thread for ever. step5, DPM device timer timeout. This patch is to delete the cur msg when msg complete. Signed-off-by: xiao jin --- drivers/spi/spi-dw.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c index d1a495f..f6bf693 100644 --- a/drivers/spi/spi-dw.c +++ b/drivers/spi/spi-dw.c @@ -268,6 +268,7 @@ static void giveback(struct dw_spi *dws) spin_lock_irqsave(>lock, flags); msg = dws->cur_msg; + list_del_init(>cur_msg->queue); dws->cur_msg = NULL; dws->cur_transfer = NULL; dws->prev_chip = dws->cur_chip; @@ -571,7 +572,6 @@ static void pump_messages(struct work_struct *work) /* Extract head of queue */ dws->cur_msg = list_entry(dws->queue.next, struct spi_message, queue); - list_del_init(>cur_msg->queue); /* Initial message state*/ dws->cur_msg->state = START_STATE; -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] spi-dw: delete cur_msg when pump message and transfer has finished
From: xiao jin jin.x...@intel.com Date: Tue, 20 Nov 2012 13:45:47 +0800 Subject: [PATCH] spi-dw: delete cur_msg when pump message and transfer has finished We see DPM device timeout happened in spi_max3111 when system suspend. The root cause is related to the time we delete msg from dw_spi queue. Below is the scenario how timeout happened. step1, max3110_main_thread pump the dw_spi msg and transfer it. And the msg is deleted from dw_spi queue right now. step2, runtime suspend happened, there is no msg in the dw_spi queue, it's permitted to enter into D0i3. step3, After resume from D0i3, irq will not come for ever, max3110_main_thread still wait for the completion of last pump message transfer. step4, system suspend, serial_m3110_shutdown call max3110_out and try to lock the mutex, but it has been hold and cannot be released by max3110_main_thread for ever. step5, DPM device timer timeout. This patch is to delete the cur msg when msg complete. Signed-off-by: xiao jin jin.x...@intel.com --- drivers/spi/spi-dw.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c index d1a495f..f6bf693 100644 --- a/drivers/spi/spi-dw.c +++ b/drivers/spi/spi-dw.c @@ -268,6 +268,7 @@ static void giveback(struct dw_spi *dws) spin_lock_irqsave(dws-lock, flags); msg = dws-cur_msg; + list_del_init(dws-cur_msg-queue); dws-cur_msg = NULL; dws-cur_transfer = NULL; dws-prev_chip = dws-cur_chip; @@ -571,7 +572,6 @@ static void pump_messages(struct work_struct *work) /* Extract head of queue */ dws-cur_msg = list_entry(dws-queue.next, struct spi_message, queue); - list_del_init(dws-cur_msg-queue); /* Initial message state*/ dws-cur_msg-state = START_STATE; -- 1.7.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] spi-dw: delete cur_msg when pump message and transfer has finished
From: xiao jin Date: Tue, 20 Nov 2012 13:45:47 +0800 Subject: [PATCH] spi-dw: delete cur_msg when pump message and transfer has finished We see DPM device timeout happened in spi_max3111 when system suspend. The root cause is related to the time we delete msg from dw_spi queue. Below is the scenario how timeout happened. step1, max3110_main_thread pump the dw_spi msg and transfer it. And the msg is deleted from dw_spi queue right now. step2, runtime suspend happened, there is no msg in the dw_spi queue, it's permitted to enter into D0i3. step3, After resume from D0i3, irq will not come for ever, max3110_main_thread still wait for the completion of last pump message transfer. step4, system suspend, serial_m3110_shutdown call max3110_out and try to lock the mutex, but it has been hold and cannot be released by max3110_main_thread for ever. step5, DPM device timer timeout. This patch is to delete the cur msg when msg complete. Signed-off-by: xiao jin --- drivers/spi/spi-dw.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c index d1a495f..f6bf693 100644 --- a/drivers/spi/spi-dw.c +++ b/drivers/spi/spi-dw.c @@ -268,6 +268,7 @@ static void giveback(struct dw_spi *dws) spin_lock_irqsave(>lock, flags); msg = dws->cur_msg; + list_del_init(>cur_msg->queue); dws->cur_msg = NULL; dws->cur_transfer = NULL; dws->prev_chip = dws->cur_chip; @@ -571,7 +572,6 @@ static void pump_messages(struct work_struct *work) /* Extract head of queue */ dws->cur_msg = list_entry(dws->queue.next, struct spi_message, queue); - list_del_init(>cur_msg->queue); /* Initial message state*/ dws->cur_msg->state = START_STATE; -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] spi-dw: delete cur_msg when pump message and transfer has finished
From: xiao jin jin.x...@intel.com Date: Tue, 20 Nov 2012 13:45:47 +0800 Subject: [PATCH] spi-dw: delete cur_msg when pump message and transfer has finished We see DPM device timeout happened in spi_max3111 when system suspend. The root cause is related to the time we delete msg from dw_spi queue. Below is the scenario how timeout happened. step1, max3110_main_thread pump the dw_spi msg and transfer it. And the msg is deleted from dw_spi queue right now. step2, runtime suspend happened, there is no msg in the dw_spi queue, it's permitted to enter into D0i3. step3, After resume from D0i3, irq will not come for ever, max3110_main_thread still wait for the completion of last pump message transfer. step4, system suspend, serial_m3110_shutdown call max3110_out and try to lock the mutex, but it has been hold and cannot be released by max3110_main_thread for ever. step5, DPM device timer timeout. This patch is to delete the cur msg when msg complete. Signed-off-by: xiao jin jin.x...@intel.com --- drivers/spi/spi-dw.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c index d1a495f..f6bf693 100644 --- a/drivers/spi/spi-dw.c +++ b/drivers/spi/spi-dw.c @@ -268,6 +268,7 @@ static void giveback(struct dw_spi *dws) spin_lock_irqsave(dws-lock, flags); msg = dws-cur_msg; + list_del_init(dws-cur_msg-queue); dws-cur_msg = NULL; dws-cur_transfer = NULL; dws-prev_chip = dws-cur_chip; @@ -571,7 +572,6 @@ static void pump_messages(struct work_struct *work) /* Extract head of queue */ dws-cur_msg = list_entry(dws-queue.next, struct spi_message, queue); - list_del_init(dws-cur_msg-queue); /* Initial message state*/ dws-cur_msg-state = START_STATE; -- 1.7.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
