[PATCH 0/2] optee: fix OOM seen due to tee_shm_free()

2021-02-18 Thread Dhananjay Phadke 
From: Allen Pais 

On Wed, 17 Feb 2021 14:57:12 +0530, Allen Pais wrote:
> The following out of memory errors are seen on kexec reboot
> from the optee core.
> 
> [0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed
> [0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22
> 
> tee_shm_release() is not invoked on dma shm buffer.
> 
> Implement .shutdown() in optee core as well as bnxt firmware driver
> to handle the release of the buffers correctly.
> 
> More info:
> https://github.com/OP-TEE/optee_os/issues/3637

CC: linux-kernel@vger.kernel.org instead of linux-mips?
TEE / TrustZone is ARM.

Also, for Broadcom specific -
CC: bcm-kernel-feedback-l...@broadcom.com

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

2020-11-13 Thread Dhananjay Phadke 
On Tue, 10 Nov 2020 11:24:36 -0800, Ray Jui wrote:

 Yes it's true that for master write-read events both
 IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
 So before the slave starts transmitting data to the master, it should
 first read all data from rx-fifo i.e. complete master write and then
 process master read.

 To minimise interrupt overhead, we are batching 64bytes.
 To keep isr running for less time, we are using a tasklet.
 Again to keep the tasklet not running for more than 20u, we have set
 max of 10 bytes data read from rx-fifo per tasklet run.

 If we start processing everything in isr and using rx threshold
 interrupt, then isr will run for a longer time and this may hog the
 system.
 For example, to process 10 bytes it takes 20us, to process 30 bytes it
 takes 60us and so on.
 So is it okay to run isr for so long ?

 Keeping all this in mind we thought a tasklet would be a good option
 and kept max of 10 bytes read per tasklet.

 Please let me know if you still feel we should not use a tasklet and
 don't batch 64 bytes.
>>>
>>> Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
>>> as i2c rate is quite low.
>>>
>
>kernel thread was proposed in the internal review. I don't see much
>benefit of using tasklet. If a thread is blocked from running for more
>than several tenth of ms, that's really a system-level issue than an
>issue with this driver.
>
>IMO, it's an overkill to use tasklet here but we can probably leave it
>as it is since it does not have a adverse effect and the code ran in
>tasklet is short.
>
>How much time is expected to read 64 bytes from an RX FIFO? Even with
>APB bus each register read is expected to be in the tenth or hundreds of
>nanosecond range. Reading the entire FIFO of 64 bytes should take less
>than 10 us. The interrupt context switch overhead is probably longer
>than that. It's much more effective to read all of them in a single
>batch than breaking them into multiple batches.

OK, there's a general discussions towards removing tasklets, if this
fix works with threaded isr, strongly recommend that route.

This comment in the code suggested that register reads take long time to
drain 64 bytes.

>+/*
>+ * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>+ * running for less time, max slave read per tasklet is set to 10
>bytes.

@Rayagonda, please take care of hand-off mentioned below, once the tasklet
is scheduled, isr should just return and clear status at the end of tasklet.

>>
>> Few other comments -
>>
>>> +  /* schedule tasklet to read data later */
>>> +  tasklet_schedule(_i2c->slave_rx_tasklet);
>>> +
>>> +  /* clear only IS_S_RX_EVENT_SHIFT interrupt */
>>> +  iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>>> +   BIT(IS_S_RX_EVENT_SHIFT));
>>> +  }
>>
>> Why clearing one rx interrupt bit here after scheduling tasklet? Should all 
>> that
>> be done by tasklet? Also should just return after scheduling tasklet?

Regards,
Dhananjay

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

2020-11-06 Thread Dhananjay Phadke 
On Thu, 5 Nov 2020 15:13:04 +0530, Rayagonda Kokatanur wrote:
>> So the suggestion was to set HW threshold for rx fifo interrupt, not
>> really a SW property. By setting it in DT, makes it easier to
>> customize for target system, module param needs or ioctl makes it
>> dependent on userpsace to configure it.
>>
>> The need for tasklet seems to arise from the fact that many bytes are
>> left in the fifo. If there's a common problem here, such tasklet would be
>> needed in i2c subsys rather than controller specific tweak, akin to
>> how networking uses NAPI or adding block transactions to the interface?
>>
>> For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
>> IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
>> drain rx fifo i.e. write is complete and the read has started on the bus?
>
>Yes it's true that for master write-read events both
>IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT  are coming together.
>So before the slave starts transmitting data to the master, it should
>first read all data from rx-fifo i.e. complete master write and then
>process master read.
>
>To minimise interrupt overhead, we are batching 64bytes.
>To keep isr running for less time, we are using a tasklet.
>Again to keep the tasklet not running for more than 20u, we have set
>max of 10 bytes data read from rx-fifo per tasklet run.
>
>If we start processing everything in isr and using rx threshold
>interrupt, then isr will run for a longer time and this may hog the
>system.
>For example, to process 10 bytes it takes 20us, to process 30 bytes it
>takes 60us and so on.
>So is it okay to run isr for so long ?
>
>Keeping all this in mind we thought a tasklet would be a good option
>and kept max of 10 bytes read per tasklet.
>
>Please let me know if you still feel we should not use a tasklet and
>don't batch 64 bytes.

Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq)
as i2c rate is quite low.

But do enable rx_threshold and read out early. This will avoid fifo full
or master write-read situation where lot of bytes must be drained from rx
fifo before serving tx fifo (avoid tx underrun).

Best would have been setting up DMA into mem (some controllers seem capable).
In absence of that, it's a trade off: if rx intr threshold is low,
there will be more interrupts, but less time spent in each. Default could
still be 64B or no-thresh (allow override in dtb).

Few other comments -

>+  /* schedule tasklet to read data later */
>+  tasklet_schedule(_i2c->slave_rx_tasklet);
>+
>+  /* clear only IS_S_RX_EVENT_SHIFT interrupt */
>+  iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
>+   BIT(IS_S_RX_EVENT_SHIFT));
>+  }

Why clearing one rx interrupt bit here after scheduling tasklet? Should all that
be done by tasklet? Also should just return after scheduling tasklet?

Thanks,
Dhananjay

Re: [PATCH v3 5/6] i2c: iproc: handle master read request

2020-11-04 Thread Dhananjay Phadke 
On Wed, 4 Nov 2020 10:01:06 -0800, Ray Jui wrote:

 +#define MAX_SLAVE_RX_PER_INT 10

>>>
 In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
 however it's not actually used in processing rx events.

 Instead of hardcoding this threshold here, it's better to add a
 device-tree knob for rx threshold, program it in controller and handle
 that RX_THLD interrupt. This will give more flexibility to drain the rx
 fifo earlier than -
 (1) waiting for FIFO_FULL interrupt for transactions > 64B.
 (2) waiting for start of read transaction in case of master write-read.
>> 
>> Yes this is one way to implement.
>> But do you see any issue in batching 64 bytes at a time in case of
>> transaction > 64 Bytes.
>> I feel batching will be more efficient as it avoids more number of
>> interrupts and hence context switch.
>> 
>>>
>>> The Device Tree is really intended to describe the hardware FIFO size,
>>> not watermarks, as those tend to be more of a policy/work load decision.
>>> Maybe this is something that can be added as a module parameter, or
>>> configurable via ioctl() at some point.
>> 
>
>Yes, DT can have properties to describe the FIFO size, if there happens
>to be some variants in the HW blocks in different versions. But that is
>not the case here. DT should not be used to control SW/use case specific
>behavior.

So the suggestion was to set HW threshold for rx fifo interrupt, not
really a SW property. By setting it in DT, makes it easier to
customize for target system, module param needs or ioctl makes it
dependent on userpsace to configure it.

The need for tasklet seems to arise from the fact that many bytes are
left in the fifo. If there's a common problem here, such tasklet would be
needed in i2c subsys rather than controller specific tweak, akin to
how networking uses NAPI or adding block transactions to the interface?

For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and
IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to
drain rx fifo i.e. write is complete and the read has started on the bus?


Thanks,
Dhananjay

[PATCH v1 5/6] i2c: iproc: handle master read request

2020-10-13 Thread Dhananjay Phadke 
On Sun, 11 Oct 2020 23:52:53 +0530, Rayagonda Kokatanur wrote:
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> 
> - } else if (status & BIT(IS_S_RD_EVENT_SHIFT)) {
> - /* Start of SMBUS for Master Read */
> + I2C_SLAVE_WRITE_REQUESTED, _data);
> + iproc_i2c->rx_start_rcvd = true;
> + iproc_i2c->slave_read_complete = false;
> + } else if (rx_status == I2C_SLAVE_RX_DATA &&
> +iproc_i2c->rx_start_rcvd) {
> + /* Middle of SMBUS Master write */
>   i2c_slave_event(iproc_i2c->slave,
> - I2C_SLAVE_READ_REQUESTED, );
> - iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
> + I2C_SLAVE_WRITE_RECEIVED, _data);
> + } else if (rx_status == I2C_SLAVE_RX_END &&
> +iproc_i2c->rx_start_rcvd) {
> + /* End of SMBUS Master write */
> + if (iproc_i2c->slave_rx_only)
> + i2c_slave_event(iproc_i2c->slave,
> + I2C_SLAVE_WRITE_RECEIVED,
> + _data);
> +
> + i2c_slave_event(iproc_i2c->slave, I2C_SLAVE_STOP,
> + _data);
> + } else if (rx_status == I2C_SLAVE_RX_FIFO_EMPTY) {
> + iproc_i2c->rx_start_rcvd = false;
> + iproc_i2c->slave_read_complete = true;
> + break;
> + }
>  
> - val = BIT(S_CMD_START_BUSY_SHIFT);
> - iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
> + rx_bytes++;

rx_bytes should be incremented only along with I2C_SLAVE_WRITE_RECEIVED event?

> 
> +static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
> + u32 status)
> +{
> + u32 val;
> + u8 value;
> +
> + /*
> +  * Slave events in case of master-write, master-write-read and,
> +  * master-read
> +  *
> +  * Master-write : only IS_S_RX_EVENT_SHIFT event
> +  * Master-write-read: both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
> +  *events
> +  * Master-read  : both IS_S_RX_EVENT_SHIFT and IS_S_RD_EVENT_SHIFT
> +  *events or only IS_S_RD_EVENT_SHIFT
> +  */
> + if (status & BIT(IS_S_RX_EVENT_SHIFT) ||
> + status & BIT(IS_S_RD_EVENT_SHIFT)) {
> + /* disable slave interrupts */
> + val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
> + val &= ~iproc_i2c->slave_int_mask;
> + iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
> +
> + if (status & BIT(IS_S_RD_EVENT_SHIFT))
> + /* Master-write-read request */
> + iproc_i2c->slave_rx_only = false;
> + else
> + /* Master-write request only */
> + iproc_i2c->slave_rx_only = true;
> +
> + /* schedule tasklet to read data later */
> + tasklet_schedule(_i2c->slave_rx_tasklet);
> +
> + /* clear only IS_S_RX_EVENT_SHIFT interrupt */
> + iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET,
> +  BIT(IS_S_RX_EVENT_SHIFT));
> 

Both tasklet and isr are writing to status (IS_OFFSET) reg.

The tasklet seems to be batching up rx fifo reads because of time-sensitive
Master-write-read transaction? Linux I2C framework is byte interface anyway.
Can the need to batch reads be avoided by setting slave rx threshold for
interrupt (S_FIFO_RX_THLD) to 1-byte? 

Also, wouldn't tasklets be susceptible to other interrupts? If fifo reads
have to be batched up, can it be changed to threaded irq?

[PATCH v3] i2c: iproc: fix race between client unreg and isr

2020-08-10 Thread Dhananjay Phadke 
When i2c client unregisters, synchronize irq before setting
iproc_i2c->slave to NULL.

(1) disable_irq()
(2) Mask event enable bits in control reg
(3) Erase slave address (avoid further writes to rx fifo)
(4) Flush tx and rx FIFOs
(5) Clear pending event (interrupt) bits in status reg
(6) enable_irq()
(7) Set client pointer to NULL

Unable to handle kernel NULL pointer dereference at virtual address 
0318

[  371.020421] pc : bcm_iproc_i2c_isr+0x530/0x11f0
[  371.025098] lr : __handle_irq_event_percpu+0x6c/0x170
[  371.030309] sp : 800010003e40
[  371.033727] x29: 800010003e40 x28: 0060
[  371.039206] x27: 800010ca9de0 x26: 800010f895df
[  371.044686] x25: 800010f1 x24: 0008f7ff3600
[  371.050165] x23: 0003 x22: 0160
[  371.055645] x21: 800010f1 x20: 0160
[  371.061124] x19: 0008f726f080 x18: 
[  371.066603] x17:  x16: 
[  371.072082] x15:  x14: 
[  371.077561] x13:  x12: 0001
[  371.083040] x11:  x10: 0040
[  371.088519] x9 : 800010f317c8 x8 : 800010f317c0
[  371.093999] x7 : 0008f805b3b0 x6 : 
[  371.099478] x5 : 0008f7ff36a4 x4 : 8008ee43d000
[  371.104957] x3 :  x2 : 8000107d64c0
[  371.110436] x1 : c0af x0 : 

[  371.115916] Call trace:
[  371.118439]  bcm_iproc_i2c_isr+0x530/0x11f0
[  371.122754]  __handle_irq_event_percpu+0x6c/0x170
[  371.127606]  handle_irq_event_percpu+0x34/0x88
[  371.132189]  handle_irq_event+0x40/0x120
[  371.136234]  handle_fasteoi_irq+0xcc/0x1a0
[  371.140459]  generic_handle_irq+0x24/0x38
[  371.144594]  __handle_domain_irq+0x60/0xb8
[  371.148820]  gic_handle_irq+0xc0/0x158
[  371.152687]  el1_irq+0xb8/0x140
[  371.155927]  arch_cpu_idle+0x10/0x18
[  371.159615]  do_idle+0x204/0x290
[  371.162943]  cpu_startup_entry+0x24/0x60
[  371.166990]  rest_init+0xb0/0xbc
[  371.170322]  arch_call_rest_init+0xc/0x14
[  371.174458]  start_kernel+0x404/0x430

Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave 
mode")

Signed-off-by: Dhananjay Phadke 
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c 
b/drivers/i2c/busses/i2c-bcm-iproc.c
index 8a3c98866fb7..688e92818821 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -1078,7 +1078,7 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client 
*slave)
if (!iproc_i2c->slave)
return -EINVAL;
 
-   iproc_i2c->slave = NULL;
+   disable_irq(iproc_i2c->irq);
 
/* disable all slave interrupts */
tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
@@ -1091,6 +1091,17 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client 
*slave)
tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);
 
+   /* flush TX/RX FIFOs */
+   tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
+   iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp);
+
+   /* clear all pending slave interrupts */
+   iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);
+
+   iproc_i2c->slave = NULL;
+
+   enable_irq(iproc_i2c->irq);
+
return 0;
 }
 
-- 
2.17.1

Re: [PATCH v2] i2c: iproc: fix race between client unreg and isr

2020-08-07 Thread Dhananjay Phadke 
On 8/7/2020, Florian Fainelli wrote:
> > When i2c client unregisters, synchronize irq before setting
> > iproc_i2c->slave to NULL.
> > 
> > (1) disable_irq()
> > (2) Mask event enable bits in control reg
> > (3) Erase slave address (avoid further writes to rx fifo)
> > (4) Flush tx and rx FIFOs
> > (5) Clear pending event (interrupt) bits in status reg
> > (6) enable_irq()
> > (7) Set client pointer to NULL
> > 
> 
> > @@ -1091,6 +1091,17 @@ static int bcm_iproc_i2c_unreg_slave(struct 
> > i2c_client *slave)
> > tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
> > iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);
> >  
> > +   /* flush TX/RX FIFOs */
> > +   tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
> > +   iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp);
> > +
> > +   /* clear all pending slave interrupts */
> > +   iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);
> > +
> > +   enable_irq(iproc_i2c->irq);
> > +
> > +   iproc_i2c->slave = NULL;
> 
> There is nothing that checks on iproc_i2c->slave being valid within the
> interrupt handler, we assume that the pointer is valid which is fin,
> however non functional it may be, it may feel more natural to move the
> assignment before the enable_irq()?

As far as the teardown sequence ensures no more interrupts arrive after
enable_irq() and they are enabled only after setting pointer during
client register(); checking for NULL in ISR isn't necessary. 

If The teardown sequence doesn't guarantee quiescing of interrupts,
setting NULL before or after enable_irq() is equally vulnerable.

Dhananjay

[PATCH v2] i2c: iproc: fix race between client unreg and isr

2020-08-07 Thread Dhananjay Phadke 
When i2c client unregisters, synchronize irq before setting
iproc_i2c->slave to NULL.

(1) disable_irq()
(2) Mask event enable bits in control reg
(3) Erase slave address (avoid further writes to rx fifo)
(4) Flush tx and rx FIFOs
(5) Clear pending event (interrupt) bits in status reg
(6) enable_irq()
(7) Set client pointer to NULL

Unable to handle kernel NULL pointer dereference at virtual address 
0318

[  371.020421] pc : bcm_iproc_i2c_isr+0x530/0x11f0
[  371.025098] lr : __handle_irq_event_percpu+0x6c/0x170
[  371.030309] sp : 800010003e40
[  371.033727] x29: 800010003e40 x28: 0060
[  371.039206] x27: 800010ca9de0 x26: 800010f895df
[  371.044686] x25: 800010f1 x24: 0008f7ff3600
[  371.050165] x23: 0003 x22: 0160
[  371.055645] x21: 800010f1 x20: 0160
[  371.061124] x19: 0008f726f080 x18: 
[  371.066603] x17:  x16: 
[  371.072082] x15:  x14: 
[  371.077561] x13:  x12: 0001
[  371.083040] x11:  x10: 0040
[  371.088519] x9 : 800010f317c8 x8 : 800010f317c0
[  371.093999] x7 : 0008f805b3b0 x6 : 
[  371.099478] x5 : 0008f7ff36a4 x4 : 8008ee43d000
[  371.104957] x3 :  x2 : 8000107d64c0
[  371.110436] x1 : c0af x0 : 

[  371.115916] Call trace:
[  371.118439]  bcm_iproc_i2c_isr+0x530/0x11f0
[  371.122754]  __handle_irq_event_percpu+0x6c/0x170
[  371.127606]  handle_irq_event_percpu+0x34/0x88
[  371.132189]  handle_irq_event+0x40/0x120
[  371.136234]  handle_fasteoi_irq+0xcc/0x1a0
[  371.140459]  generic_handle_irq+0x24/0x38
[  371.144594]  __handle_domain_irq+0x60/0xb8
[  371.148820]  gic_handle_irq+0xc0/0x158
[  371.152687]  el1_irq+0xb8/0x140
[  371.155927]  arch_cpu_idle+0x10/0x18
[  371.159615]  do_idle+0x204/0x290
[  371.162943]  cpu_startup_entry+0x24/0x60
[  371.166990]  rest_init+0xb0/0xbc
[  371.170322]  arch_call_rest_init+0xc/0x14
[  371.174458]  start_kernel+0x404/0x430

Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave 
mode")

Signed-off-by: Dhananjay Phadke 
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c 
b/drivers/i2c/busses/i2c-bcm-iproc.c
index 8a3c98866fb7..c576776ffb10 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -1078,7 +1078,7 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client 
*slave)
if (!iproc_i2c->slave)
return -EINVAL;
 
-   iproc_i2c->slave = NULL;
+   disable_irq(iproc_i2c->irq);
 
/* disable all slave interrupts */
tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
@@ -1091,6 +1091,17 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client 
*slave)
tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);
 
+   /* flush TX/RX FIFOs */
+   tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
+   iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp);
+
+   /* clear all pending slave interrupts */
+   iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);
+
+   enable_irq(iproc_i2c->irq);
+
+   iproc_i2c->slave = NULL;
+
return 0;
 }
 
-- 
2.17.1

Re: [PATCH] i2c: iproc: fix race between client unreg and isr

2020-08-07 Thread Dhananjay Phadke 
Ray Jui wrote:
> 
> Any progress yet?
> 

> I don't know if Dhananjay is actively working on this or not.
> 
> Rayagonda, given that you have the I2C slave setup already, do you think
> you can help to to test and above sequence from Wolfram (by using the
> widened delay window as instructed)?
> 
> Thanks,
> 
> Ray

Sorry I was out for a while, I've tested the fix with delay and original
i2c client test, will send v2 patch shortly.

Thanks,
Dhananjay

Re: [PATCH] i2c: iproc: fix race between client unreg and isr

2020-07-27 Thread Dhananjay Phadke 
Ray Jui  wrote:
>>> I think the following sequence needs to be implemented to make this
>>> safe, i.e., after 'synchronize_irq', no further slave interrupt will be
>>> fired.
>>>
>>> In 'bcm_iproc_i2c_unreg_slave':
>>>
>>> 1. Set an atomic variable 'unreg_slave' (I'm bad in names so please come
>>> up with a better name than this)
>>>
>>> 2. Disable all slave interrupts
>>>
>>> 3. synchronize_irq
>>>
>>> 4. Set slave to NULL
>>>
>>> 5. Erase slave addresses
>> 
>> What about this in unreg_slave?
>> 
>> 1. disable_irq()
>>  This includes synchronize_irq() and avoids the race. Because irq
>>  will be masked at interrupt controller level, interrupts coming
>>  in at the I2C IP core level should still be pending once we
>>  reenable the irq.
>> 
> 
> Can you confirm that even if we have irq pending at the i2c IP core
> level, as long as we execute Step 2. below (to disable/mask all slave
> interrupts), after 'enable_irq' is called, we still will not receive any
> further i2c slave interrupt?
> 
> Basically I'm asking if interrupts will be "cached" at the GIC
> controller level after 'disable_irq' is called. As long as that is not
> the case, then I think we are good.
> 
> The goal of course is to ensure there's no further slave interrupts
> after 'enable_irq' in Step 3 below.

That was my question as well, the best would be if the i2c controller itself
has a bit for masking all interrupts overriding individual event enables
set by the ISR.

Also with regards to the original sequence, I think slave address should
be erased before enable_irq(), besides draining rx and tx FIFOs.

I'll send reworked patch.

@Rayagonda will validate new sequence with the test that hit the race condition.

- Dhananjay

Re: [PATCH] i2c: iproc: fix race between client unreg and isr

2020-07-22 Thread Dhananjay Phadke 
Ray Jui  wrote:

>
> On 7/22/2020 3:41 AM, Wolfram Sang wrote:
> > 
> >>> + synchronize_irq(iproc_i2c->irq);
> >>
> >> If one takes a look at the I2C slave ISR routine, there are places where
> >> IRQ can be re-enabled in the ISR itself. What happens after we mask all
> >> slave interrupt and when 'synchronize_irq' is called, which I suppose is
> >> meant to wait for inflight interrupt to finish where there's a chance
> >> the interrupt can be re-enable again? How is one supposed to deal with 
> >> that?
> > 
> > I encountered the same problem with the i2c-rcar driver before I left
> > for my holidays.
> > 
>
> I think the following sequence needs to be implemented to make this
> safe, i.e., after 'synchronize_irq', no further slave interrupt will be
> fired.
>
> In 'bcm_iproc_i2c_unreg_slave':
>
> 1. Set an atomic variable 'unreg_slave' (I'm bad in names so please come
> up with a better name than this)
>
> 2. Disable all slave interrupts
>
> 3. synchronize_irq
>
> 4. Set slave to NULL
>
> 5. Erase slave addresses
>
> In the ISR routine, it should always check against 'unreg_slave' before
> enabling any slave interrupt. If 'unreg_slave' is set, no slave
> interrupt should be re-enabled from within the ISR.
>
> I think the above sequence can ensure no further slave interrupt after
> 'synchronize_irq'. I suggested using an atomic variable instead of
> variable + spinlock due to the way how sync irq works, i.e., "If you use
> this function while holding a resource the IRQ handler may need you will
> deadlock.".
>
> Thanks,
>
> Ray
>
> >>> + iproc_i2c->slave = NULL;
> >>> +
> >>>   /* Erase the slave address programmed */
> >>>   tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
> >>>   tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
> >>>

[PATCH] i2c: iproc: fix race between client unreg and isr

2020-07-18 Thread Dhananjay Phadke 
When i2c client unregisters, synchronize irq before setting
iproc_i2c->slave to NULL.

Unable to handle kernel NULL pointer dereference at virtual address 
0318

[  371.020421] pc : bcm_iproc_i2c_isr+0x530/0x11f0
[  371.025098] lr : __handle_irq_event_percpu+0x6c/0x170
[  371.030309] sp : 800010003e40
[  371.033727] x29: 800010003e40 x28: 0060
[  371.039206] x27: 800010ca9de0 x26: 800010f895df
[  371.044686] x25: 800010f1 x24: 0008f7ff3600
[  371.050165] x23: 0003 x22: 0160
[  371.055645] x21: 800010f1 x20: 0160
[  371.061124] x19: 0008f726f080 x18: 
[  371.066603] x17:  x16: 
[  371.072082] x15:  x14: 
[  371.077561] x13:  x12: 0001
[  371.083040] x11:  x10: 0040
[  371.088519] x9 : 800010f317c8 x8 : 800010f317c0
[  371.093999] x7 : 0008f805b3b0 x6 : 
[  371.099478] x5 : 0008f7ff36a4 x4 : 8008ee43d000
[  371.104957] x3 :  x2 : 8000107d64c0
[  371.110436] x1 : c0af x0 : 

[  371.115916] Call trace:
[  371.118439]  bcm_iproc_i2c_isr+0x530/0x11f0
[  371.122754]  __handle_irq_event_percpu+0x6c/0x170
[  371.127606]  handle_irq_event_percpu+0x34/0x88
[  371.132189]  handle_irq_event+0x40/0x120
[  371.136234]  handle_fasteoi_irq+0xcc/0x1a0
[  371.140459]  generic_handle_irq+0x24/0x38
[  371.144594]  __handle_domain_irq+0x60/0xb8
[  371.148820]  gic_handle_irq+0xc0/0x158
[  371.152687]  el1_irq+0xb8/0x140
[  371.155927]  arch_cpu_idle+0x10/0x18
[  371.159615]  do_idle+0x204/0x290
[  371.162943]  cpu_startup_entry+0x24/0x60
[  371.166990]  rest_init+0xb0/0xbc
[  371.170322]  arch_call_rest_init+0xc/0x14
[  371.174458]  start_kernel+0x404/0x430

Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave 
mode")
Signed-off-by: Dhananjay Phadke 
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c 
b/drivers/i2c/busses/i2c-bcm-iproc.c
index b58224b7b..37d2a79e7 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -1074,14 +1074,15 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client 
*slave)
if (!iproc_i2c->slave)
return -EINVAL;
 
-   iproc_i2c->slave = NULL;
-
/* disable all slave interrupts */
tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
IE_S_ALL_INTERRUPT_SHIFT);
iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
 
+   synchronize_irq(iproc_i2c->irq);
+   iproc_i2c->slave = NULL;
+
/* Erase the slave address programmed */
tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);

Re: [2.6 patch] drivers/net/netxen/: cleanups

2007-11-08 Thread Dhananjay Phadke 
This looks good to me, I might chop off these #if 0 'ed functions in
my next round of patches.

Acked-by: Dhananjay Phadke <[EMAIL PROTECTED]>

On 11/5/07, Adrian Bunk <[EMAIL PROTECTED]> wrote:
> This patch contains the following cleanups:
> - static functions in .c files shouldn't be marked inline
> - make needlessly global code static
> - #if 0 unused code
>
> Signed-off-by: Adrian Bunk <[EMAIL PROTECTED]>
>
> ---
>
>  drivers/net/netxen/netxen_nic.h  |   14 
>  drivers/net/netxen/netxen_nic_hw.c   |   19 +++---
>  drivers/net/netxen/netxen_nic_hw.h   |8 --
>  drivers/net/netxen/netxen_nic_init.c |   70 ---
>  drivers/net/netxen/netxen_nic_isr.c  |7 +-
>  drivers/net/netxen/netxen_nic_main.c |9 +-
>  drivers/net/netxen/netxen_nic_niu.c  |   26 +---
>  drivers/net/netxen/netxen_nic_phan_reg.h |7 --
>  8 files changed, 76 insertions(+), 84 deletions(-)
>
> dbc7aeed37e41cd37a01cce259e5c0ab01f8dd88
> diff --git a/drivers/net/netxen/netxen_nic.h b/drivers/net/netxen/netxen_nic.h
> index fbc2553..ef9f986 100644
> --- a/drivers/net/netxen/netxen_nic.h
> +++ b/drivers/net/netxen/netxen_nic.h
> @@ -1015,14 +1015,8 @@ int netxen_niu_xgbe_enable_phy_interrupts(struct 
> netxen_adapter *adapter);
>  int netxen_niu_gbe_enable_phy_interrupts(struct netxen_adapter *adapter);
>  int netxen_niu_xgbe_disable_phy_interrupts(struct netxen_adapter *adapter);
>  int netxen_niu_gbe_disable_phy_interrupts(struct netxen_adapter *adapter);
> -int netxen_niu_xgbe_clear_phy_interrupts(struct netxen_adapter *adapter);
> -int netxen_niu_gbe_clear_phy_interrupts(struct netxen_adapter *adapter);
>  void netxen_nic_xgbe_handle_phy_intr(struct netxen_adapter *adapter);
>  void netxen_nic_gbe_handle_phy_intr(struct netxen_adapter *adapter);
> -void netxen_niu_gbe_set_mii_mode(struct netxen_adapter *adapter, int port,
> -long enable);
> -void netxen_niu_gbe_set_gmii_mode(struct netxen_adapter *adapter, int port,
> - long enable);
>  int netxen_niu_gbe_phy_read(struct netxen_adapter *adapter, long reg,
> __u32 * readval);
>  int netxen_niu_gbe_phy_write(struct netxen_adapter *adapter,
> @@ -1045,7 +1039,6 @@ int netxen_nic_hw_write_wx(struct netxen_adapter 
> *adapter, u64 off, void *data,
>int len);
>  void netxen_crb_writelit_adapter(struct netxen_adapter *adapter,
>  unsigned long off, int data);
> -int netxen_nic_erase_pxe(struct netxen_adapter *adapter);
>
>  /* Functions from netxen_nic_init.c */
>  void netxen_free_adapter_offload(struct netxen_adapter *adapter);
> @@ -1064,15 +1057,10 @@ int netxen_flash_erase_secondary(struct 
> netxen_adapter *adapter);
>  int netxen_flash_erase_primary(struct netxen_adapter *adapter);
>  void netxen_halt_pegs(struct netxen_adapter *adapter);
>
> -int netxen_rom_fast_write(struct netxen_adapter *adapter, int addr, int 
> data);
>  int netxen_rom_se(struct netxen_adapter *adapter, int addr);
> -int netxen_do_rom_se(struct netxen_adapter *adapter, int addr);
>
>  /* Functions from netxen_nic_isr.c */
>  int netxen_nic_link_ok(struct netxen_adapter *adapter);
> -void netxen_nic_isr_other(struct netxen_adapter *adapter);
> -void netxen_indicate_link_status(struct netxen_adapter *adapter, u32 link);
> -void netxen_handle_port_int(struct netxen_adapter *adapter, u32 enable);
>  void netxen_initialize_adapter_sw(struct netxen_adapter *adapter);
>  void netxen_initialize_adapter_hw(struct netxen_adapter *adapter);
>  void *netxen_alloc(struct pci_dev *pdev, size_t sz, dma_addr_t * ptr,
> @@ -1089,8 +1077,6 @@ int netxen_nic_tx_has_work(struct netxen_adapter 
> *adapter);
>  void netxen_watchdog_task(struct work_struct *work);
>  void netxen_post_rx_buffers(struct netxen_adapter *adapter, u32 ctx,
> u32 ringid);
> -void netxen_post_rx_buffers_nodb(struct netxen_adapter *adapter, u32 ctx,
> -u32 ringid);
>  int netxen_process_cmd_ring(unsigned long data);
>  u32 netxen_process_rcv_ring(struct netxen_adapter *adapter, int ctx, int 
> max);
>  void netxen_nic_set_multi(struct net_device *netdev);
> diff --git a/drivers/net/netxen/netxen_nic_hw.c 
> b/drivers/net/netxen/netxen_nic_hw.c
> index 2c19b8d..b2c7861 100644
> --- a/drivers/net/netxen/netxen_nic_hw.c
> +++ b/drivers/net/netxen/netxen_nic_hw.c
> @@ -33,7 +33,6 @@
>
>  #include "netxen_nic.h"
>  #include "netxen_nic_hw.h"
> -#define DEFINE_GLOBAL_RECV_CRB
>  #include "netxen_nic_phan_reg.h"
>
>
> @@ -244,12 +243,15 @@ struct netxen_recv_crb recv_cr

Re: [2.6 patch] drivers/net/netxen/: cleanups

2007-11-08 Thread Dhananjay Phadke 
This looks good to me, I might chop off these #if 0 'ed functions in
my next round of patches.

Acked-by: Dhananjay Phadke [EMAIL PROTECTED]

On 11/5/07, Adrian Bunk [EMAIL PROTECTED] wrote:
 This patch contains the following cleanups:
 - static functions in .c files shouldn't be marked inline
 - make needlessly global code static
 - #if 0 unused code

 Signed-off-by: Adrian Bunk [EMAIL PROTECTED]

 ---

  drivers/net/netxen/netxen_nic.h  |   14 
  drivers/net/netxen/netxen_nic_hw.c   |   19 +++---
  drivers/net/netxen/netxen_nic_hw.h   |8 --
  drivers/net/netxen/netxen_nic_init.c |   70 ---
  drivers/net/netxen/netxen_nic_isr.c  |7 +-
  drivers/net/netxen/netxen_nic_main.c |9 +-
  drivers/net/netxen/netxen_nic_niu.c  |   26 +---
  drivers/net/netxen/netxen_nic_phan_reg.h |7 --
  8 files changed, 76 insertions(+), 84 deletions(-)

 dbc7aeed37e41cd37a01cce259e5c0ab01f8dd88
 diff --git a/drivers/net/netxen/netxen_nic.h b/drivers/net/netxen/netxen_nic.h
 index fbc2553..ef9f986 100644
 --- a/drivers/net/netxen/netxen_nic.h
 +++ b/drivers/net/netxen/netxen_nic.h
 @@ -1015,14 +1015,8 @@ int netxen_niu_xgbe_enable_phy_interrupts(struct 
 netxen_adapter *adapter);
  int netxen_niu_gbe_enable_phy_interrupts(struct netxen_adapter *adapter);
  int netxen_niu_xgbe_disable_phy_interrupts(struct netxen_adapter *adapter);
  int netxen_niu_gbe_disable_phy_interrupts(struct netxen_adapter *adapter);
 -int netxen_niu_xgbe_clear_phy_interrupts(struct netxen_adapter *adapter);
 -int netxen_niu_gbe_clear_phy_interrupts(struct netxen_adapter *adapter);
  void netxen_nic_xgbe_handle_phy_intr(struct netxen_adapter *adapter);
  void netxen_nic_gbe_handle_phy_intr(struct netxen_adapter *adapter);
 -void netxen_niu_gbe_set_mii_mode(struct netxen_adapter *adapter, int port,
 -long enable);
 -void netxen_niu_gbe_set_gmii_mode(struct netxen_adapter *adapter, int port,
 - long enable);
  int netxen_niu_gbe_phy_read(struct netxen_adapter *adapter, long reg,
 __u32 * readval);
  int netxen_niu_gbe_phy_write(struct netxen_adapter *adapter,
 @@ -1045,7 +1039,6 @@ int netxen_nic_hw_write_wx(struct netxen_adapter 
 *adapter, u64 off, void *data,
int len);
  void netxen_crb_writelit_adapter(struct netxen_adapter *adapter,
  unsigned long off, int data);
 -int netxen_nic_erase_pxe(struct netxen_adapter *adapter);

  /* Functions from netxen_nic_init.c */
  void netxen_free_adapter_offload(struct netxen_adapter *adapter);
 @@ -1064,15 +1057,10 @@ int netxen_flash_erase_secondary(struct 
 netxen_adapter *adapter);
  int netxen_flash_erase_primary(struct netxen_adapter *adapter);
  void netxen_halt_pegs(struct netxen_adapter *adapter);

 -int netxen_rom_fast_write(struct netxen_adapter *adapter, int addr, int 
 data);
  int netxen_rom_se(struct netxen_adapter *adapter, int addr);
 -int netxen_do_rom_se(struct netxen_adapter *adapter, int addr);

  /* Functions from netxen_nic_isr.c */
  int netxen_nic_link_ok(struct netxen_adapter *adapter);
 -void netxen_nic_isr_other(struct netxen_adapter *adapter);
 -void netxen_indicate_link_status(struct netxen_adapter *adapter, u32 link);
 -void netxen_handle_port_int(struct netxen_adapter *adapter, u32 enable);
  void netxen_initialize_adapter_sw(struct netxen_adapter *adapter);
  void netxen_initialize_adapter_hw(struct netxen_adapter *adapter);
  void *netxen_alloc(struct pci_dev *pdev, size_t sz, dma_addr_t * ptr,
 @@ -1089,8 +1077,6 @@ int netxen_nic_tx_has_work(struct netxen_adapter 
 *adapter);
  void netxen_watchdog_task(struct work_struct *work);
  void netxen_post_rx_buffers(struct netxen_adapter *adapter, u32 ctx,
 u32 ringid);
 -void netxen_post_rx_buffers_nodb(struct netxen_adapter *adapter, u32 ctx,
 -u32 ringid);
  int netxen_process_cmd_ring(unsigned long data);
  u32 netxen_process_rcv_ring(struct netxen_adapter *adapter, int ctx, int 
 max);
  void netxen_nic_set_multi(struct net_device *netdev);
 diff --git a/drivers/net/netxen/netxen_nic_hw.c 
 b/drivers/net/netxen/netxen_nic_hw.c
 index 2c19b8d..b2c7861 100644
 --- a/drivers/net/netxen/netxen_nic_hw.c
 +++ b/drivers/net/netxen/netxen_nic_hw.c
 @@ -33,7 +33,6 @@

  #include netxen_nic.h
  #include netxen_nic_hw.h
 -#define DEFINE_GLOBAL_RECV_CRB
  #include netxen_nic_phan_reg.h


 @@ -244,12 +243,15 @@ struct netxen_recv_crb recv_crb_registers[] = {
 },
  };

 -u64 ctx_addr_sig_regs[][3] = {
 +static u64 ctx_addr_sig_regs[][3] = {
 {NETXEN_NIC_REG(0x188), NETXEN_NIC_REG(0x18c), NETXEN_NIC_REG(0x1c0)},
 {NETXEN_NIC_REG(0x190), NETXEN_NIC_REG(0x194), NETXEN_NIC_REG(0x1c4)},
 {NETXEN_NIC_REG(0x198), NETXEN_NIC_REG(0x19c), NETXEN_NIC_REG(0x1c8)},
 {NETXEN_NIC_REG(0x1a0), NETXEN_NIC_REG(0x1a4), NETXEN_NIC_REG(0x1cc

Re: NetXen driver causing slab corruption in -RT kernels

2007-09-19 Thread Dhananjay Phadke 
That "00 0e 1e ..." is netxen's mac address, so sounds like the NIC is
dumping a frame in the skb already freed (and poisoned) by the stack.
I suppose -RT kernels preempt the softirq, giving a chance for this race.

The netxen driver doesn't seem to clear the mapped address of the skb in
rx descriptor, instead it replaces it with mapped address of newly
allocated skb. Also, the rx ring is replenished in bursts (at the end of
poll routine), this can help the race if preempted in between.

I am currently reworking the rx handling, hopefully it will fix this.

Thanks for reporting in detail.

-Dhananjay


Vernon Mauery wrote:
> In doing some stress testing of the NetXen driver, I found that my machine 
> was 
> dying in all sorts of weird ways.  I saw several different crashes, BUG 
> messages in the TCP stack and some assert messages in the TCP stack as well.  
> I really didn't think that there could be six different bugs all at once in 
> the TCP/IP stack, so I started looking at possible memory corruption.
> 
> I first saw this on 2.6.16-rt22 with a backported netxen driver from 2.6.22.  
> I figured I should try the latest kernel, so I tried it on 2.6.23-rc6-git7 
> but could not trigger the slab corruption messages with CONFIG_DEBUG_SLAB, so 
> I figured the race must only exist in the -RT kernels.  Next I tried 
> 2.6.23-rc6-git7-rt1 (I applied patch-2.6.23-rc4-rt1 patch to 2.6.23-rc6-git7 
> and fixed the 5 failing hunks).  After an hour or so, lots of slab corruption 
> messages showed up:
> 
> Slab corruption: size-2048 start=f40c4670, len=2048
> Slab corruption: size-2048 start=f313cf48, len=2048
> Redzone: 0x9f911029d74e35b/0x9f911029d74e35b.
> Last user: [](kfree+0x80/0x95)
> 010: 6b 6b 00 0e 1e 00 16 13 00 0e 1e 00 19 3d 08 00
> 020: 45 00 05 dc 92 ab 40 00 40 11 8a 5b 0a 02 02 03
> 030: 0a 02 02 04 80 0c 80 0d 05 c8 dc 39 00 00 00 00
> 040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Prev obj: start=f313c730, len=2048
> Redzone: 0xd84156c5635688c0/0xd84156c5635688c0.
> Last user: [](netxen_post_rx_buffers_nodb+0x62/0x1f0 [netxen_nic])
> 000: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
> 010: 5a 5a 00 0e 1e 00 16 13 00 0e 1e 00 19 3d 08 00
> Next obj: start=f313d760, len=2048
> Redzone: 0xd84156c5635688c0/0xd84156c5635688c0.
> Last user: [](netxen_post_rx_buffers_nodb+0x62/0x1f0 [netxen_nic])
> 000: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
> 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
> Slab corruption: size-2048 start=f395a6f0, len=2048
> Redzone: 0x9f911029d74e35b/0x9f911029d74e35b.
> Last user: [](kfree+0x80/0x95)
> 010: 6b 6b 00 0e 1e 00 16 13 00 0e 1e 00 19 3d 08 00
> 020: 45 00 05 dc 92 ac 40 00 40 11 8a 5a 0a 02 02 03
> 030: 0a 02 02 04 80 0c 80 0d 05 c8 dc 39 00 00 00 00
> 040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Next obj: start=f395af08, len=2048
> Redzone: 0xd84156c5635688c0/0xd84156c5635688c0.
> Last user: [](netxen_post_rx_buffers_nodb+0x62/0x1f0 [netxen_nic])
> 000: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
> 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
> Redzone: 0x9f911029d74e35b/0x9f911029d74e35b.
> Last user: [](kfree+0x80/0x95)
> 010: 6b 6b 00 0e 1e 00 16 13 00 0e 1e 00 19 3d 08 00
> 020: 45 00 05 dc 92 aa 40 00 40 11 8a 5c 0a 02 02 03
> 030: 0a 02 02 04 80 0c 80 0d 05 c8 dc 39 00 00 00 00
> 040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Next obj: start=f40c4e88, len=2048
> Redzone: 0xd84156c5635688c0/0xd84156c5635688c0.
> Last user: [](netxen_post_rx_buffers_nodb+0x62/0x1f0 [netxen_nic])
> 000: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
> 010: 5a 5a 00 0e 1e 00 16 13 00 0e 1e 00 19 3d 08 00
> 
> The stress test that I am running is basically a mixed bag of stuff I threw 
> together.  It runs eight concurrent netperf TCP streams and two concurrent 
> UDP streams in both directions, (and on both 10GbE interfaces), ping -f in 
> both directions, some more disk/cpu loads in the background and a little bit 
> of NFS traffic thrown in for good measure.
> 
> --Vernon
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NetXen driver causing slab corruption in -RT kernels

2007-09-19 Thread Dhananjay Phadke 
That 00 0e 1e ... is netxen's mac address, so sounds like the NIC is
dumping a frame in the skb already freed (and poisoned) by the stack.
I suppose -RT kernels preempt the softirq, giving a chance for this race.

The netxen driver doesn't seem to clear the mapped address of the skb in
rx descriptor, instead it replaces it with mapped address of newly
allocated skb. Also, the rx ring is replenished in bursts (at the end of
poll routine), this can help the race if preempted in between.

I am currently reworking the rx handling, hopefully it will fix this.

Thanks for reporting in detail.

-Dhananjay


Vernon Mauery wrote:
 In doing some stress testing of the NetXen driver, I found that my machine 
 was 
 dying in all sorts of weird ways.  I saw several different crashes, BUG 
 messages in the TCP stack and some assert messages in the TCP stack as well.  
 I really didn't think that there could be six different bugs all at once in 
 the TCP/IP stack, so I started looking at possible memory corruption.
 
 I first saw this on 2.6.16-rt22 with a backported netxen driver from 2.6.22.  
 I figured I should try the latest kernel, so I tried it on 2.6.23-rc6-git7 
 but could not trigger the slab corruption messages with CONFIG_DEBUG_SLAB, so 
 I figured the race must only exist in the -RT kernels.  Next I tried 
 2.6.23-rc6-git7-rt1 (I applied patch-2.6.23-rc4-rt1 patch to 2.6.23-rc6-git7 
 and fixed the 5 failing hunks).  After an hour or so, lots of slab corruption 
 messages showed up:
 
 Slab corruption: size-2048 start=f40c4670, len=2048
 Slab corruption: size-2048 start=f313cf48, len=2048
 Redzone: 0x9f911029d74e35b/0x9f911029d74e35b.
 Last user: [c0166be4](kfree+0x80/0x95)
 010: 6b 6b 00 0e 1e 00 16 13 00 0e 1e 00 19 3d 08 00
 020: 45 00 05 dc 92 ab 40 00 40 11 8a 5b 0a 02 02 03
 030: 0a 02 02 04 80 0c 80 0d 05 c8 dc 39 00 00 00 00
 040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 Prev obj: start=f313c730, len=2048
 Redzone: 0xd84156c5635688c0/0xd84156c5635688c0.
 Last user: [f8f06186](netxen_post_rx_buffers_nodb+0x62/0x1f0 [netxen_nic])
 000: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
 010: 5a 5a 00 0e 1e 00 16 13 00 0e 1e 00 19 3d 08 00
 Next obj: start=f313d760, len=2048
 Redzone: 0xd84156c5635688c0/0xd84156c5635688c0.
 Last user: [f8f06186](netxen_post_rx_buffers_nodb+0x62/0x1f0 [netxen_nic])
 000: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
 Slab corruption: size-2048 start=f395a6f0, len=2048
 Redzone: 0x9f911029d74e35b/0x9f911029d74e35b.
 Last user: [c0166be4](kfree+0x80/0x95)
 010: 6b 6b 00 0e 1e 00 16 13 00 0e 1e 00 19 3d 08 00
 020: 45 00 05 dc 92 ac 40 00 40 11 8a 5a 0a 02 02 03
 030: 0a 02 02 04 80 0c 80 0d 05 c8 dc 39 00 00 00 00
 040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 Next obj: start=f395af08, len=2048
 Redzone: 0xd84156c5635688c0/0xd84156c5635688c0.
 Last user: [f8f06186](netxen_post_rx_buffers_nodb+0x62/0x1f0 [netxen_nic])
 000: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
 Redzone: 0x9f911029d74e35b/0x9f911029d74e35b.
 Last user: [c0166be4](kfree+0x80/0x95)
 010: 6b 6b 00 0e 1e 00 16 13 00 0e 1e 00 19 3d 08 00
 020: 45 00 05 dc 92 aa 40 00 40 11 8a 5c 0a 02 02 03
 030: 0a 02 02 04 80 0c 80 0d 05 c8 dc 39 00 00 00 00
 040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 Next obj: start=f40c4e88, len=2048
 Redzone: 0xd84156c5635688c0/0xd84156c5635688c0.
 Last user: [f8f06186](netxen_post_rx_buffers_nodb+0x62/0x1f0 [netxen_nic])
 000: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a
 010: 5a 5a 00 0e 1e 00 16 13 00 0e 1e 00 19 3d 08 00
 
 The stress test that I am running is basically a mixed bag of stuff I threw 
 together.  It runs eight concurrent netperf TCP streams and two concurrent 
 UDP streams in both directions, (and on both 10GbE interfaces), ping -f in 
 both directions, some more disk/cpu loads in the background and a little bit 
 of NFS traffic thrown in for good measure.
 
 --Vernon
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/