On 10/10/19 8:31 PM, Jeroen Hofstee wrote:
>> drivers/net/can/ti_hecc.c | 34 +++++++++++++++++++++++++++++-----
>> 1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
>> index 6ea29126c60b..87815a5b9170 100644
>> --- a/drivers/net/can/ti_hecc.c
>> +++ b/drivers/net/can/ti_hecc.c
[...]
>> @@ -531,8 +539,22 @@ static unsigned int ti_hecc_mailbox_read(struct can_rx_offload *offload,
>> {
>> struct ti_hecc_priv *priv = rx_offload_to_priv(offload);
>> u32 data, mbx_mask;
>> + int ret = 1;
>>
>> mbx_mask = BIT(mbxno);
>> +
>> + /* check if the last mailbox has been overwritten */
>> + if (unlikely(mbxno == HECC_RX_LAST_MBOX)) {
>> + data = hecc_read(priv, HECC_CANRML);
>> + if (unlikely(data & mbx_mask)) {
>> + offload->dev->stats.rx_over_errors++;
>> + offload->dev->stats.rx_errors++;
>> +
>> + ret = 0;
>> + goto mark_ask_read;
>> + }
>> + }
>> +
>
>
> This isn't right though, during the code below the msg might,
> get overwritten, so we can end up with a corrupt message.
> (as in canId from one, and the data from the overwritten one).
> That is not good. And I did actually test that to make sure
> v1 didn't have such a race. (it is rather easy to actually trigger).
I'm doing the check at the end now.
> I really doubt if there is something to justify to treat the
> HECC_RX_LAST_MBOX specially. It is a comparison versus a bit
> check of which the mask is already available.....
You miss the point that we will read the HECC_CANRML register for
_every_ mailbox and reading a register from a peripheral is much more
expensive then first checking if we're working on the last RX mailbox at
all.
> I like v1 more actually, just because it is simpler / less code...
v3 looks quite nice now.
> I have my doubts about the error counter, see the other mail.
> I don't know which source has the ultimate definition, I just
> googled for them.
I've moved the error counter handling to rx-offload now.
The kernel doc always says:
"See the network driver for the exact meaning of this value."
https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-class-net-statistics#L54
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Attachment:
signature.asc
Description: OpenPGP digital signature
