Wolfgang Grandegger wrote: > On 07/25/2010 05:43 PM, Marc Kleine-Budde wrote: >> Wolfgang Grandegger wrote: >>> On 07/25/2010 04:48 PM, Marc Kleine-Budde wrote: > ... >>>> I like that idea! From naive point of view, in the userspace I want >>>> information that I can interpret stateless (i.e. without the need for a >>>> statemachine == history). >>> I tend to introduce the error class: >>> >>> #define CAN_ERR_STATE_CHANGE 0x00000200U /* state changed / data[1] >>> >>> apart from >>> >>> #define CAN_ERR_CRTL 0x00000004U /* controller problems / data[1] >>> >>> For any state change the CAN_ERR_STATE_CHANGE bit is set. If the state >>> got worse, the CAN_ERR_CRTL bit will be set as well. This would also >>> ensure backward compatibility.
sounds good!
>> What about CAN_CTRLMODE_BERR_REPORTING? How strict should it be
>> interpreted? In the flexcan driver, if CAN_CTRLMODE_BERR_REPORTING is
>> not active I don't send any bus error related error frames. Even if the
>> napi function is entered and the bus error bits are set!
>
> Well, CAN_CTRLMODE_BERR_REPORTING was introduced to reduce the CPU load
> due to bus errors by instructing the hardware to enable/disable bus
> error interrupts. If that's not possible, there is no need to support
> CAN_CTRLMODE_BERR_REPORTING. It does not tell if bus errors can be
> generated by the hardware, at least that was the initial idea. I
> realized that it's used in the Flexcan driver to suppress sending error
> frames upstream, which is OK. I will update the ESD USB2 driver accordingly.
This way it reduces the cpu load on the flexcan driver, too. :)
> Anyway, it's not related to the state change handling ;-).
....*looking at code*...yes, you're right. I messed that up.
>>>>> No need for another state machine. But a common function is still useful
>>>>> to fill an error frame for the state change, of course. I will try to
>>>>> come up with a proposal soon.
>>>> I think there's need for (another) state machine, let me try to explain.
>>>>
>>>> In the interrupt or napi handler each driver determines the current can
>>>> state.
>>>> This can vary from a simple: read register and translate into
>>>> CAN_STATE_*, to a more complex function like you've written above. This
>>>> functions can even have fixups for strange hardware, like the MSCAN: The
>>>> mscan may think it's still in error warning, but we know tx/rx error
>>>> level below 96 means error active.
>>>>
>>>> If the can state has changed the driver calls the
>>>> "fill-out-error-frame-function". This function will take care of strange
>>>> hardware again. E.g. if the can state changes from error passive to
>>>> error active the function can generate the missing to-error-warning
>>>> meesage. (If this is wanted). The same goes for the
>>>> state-is-getting-worse messages.
>>> OK, the prototype of a common function could be (similar to your
>>> do_state function in the flexcan driver):
>>>
>>> int can_handle_state_change(struct net_device *dev,
>>> struct can_frame *cf,
>>> enum can_state new_state,
>>> int tx_err, int rx_err)
>> I'd drop the "struct can_frame *cf", the function can take care of
>> allocating the frame and returning in case of an error. But this would
>> break you proposed detect "getting worse".
Sorry, /me was confused again.
> Why, the calling function needs to allocate the frame anyway. Filling
Depends, flexcan has two functions, one for bus errors, the other for
state changes.....but this can be merged into a single function.
> the error message is one of the major tasks of that function. I would
> also like to add tx- and rx-errs to data[6..7].
sure
>> Apart from this, {tx,rx}_err can be derived via the
>> priv->can.do_get_berr_counter function.
>
> Yes, I agree. And if it's "NULL", it cannot be read.
ACK
>>> If "tx_err" or "rx_err == -1", they are ignored. If "new_state ==
>> If priv->can.do_get_berr_counter if NULL, don't use it.
>>> CAN_STATE_UNKNOWN", the state could be derived from tx_err or rx_err.
>>> What do you think?
>> The CAN_STATE_UNKNOWN fallback is a good idea.
>
> Let's see if we really need it.
ACK
> I looked to your Flexcan implementation and I actually don't like that
> more than one state might be set in the error message. It just makes it
> tricky for the user space to handle it properly. The error message
> reports a state change to "x" and it does not matter, if it does not
> report state changes in between. What do you think?
From the make-it-easy-for-the-userspace point of view this makes
perfectly sense!
Two things about "tx- and rx-errs to data[6..7]." One of these can have
a value of 0x100 (IIRC it was the tx), that would not fit into an u8.
There are controllers, which are buggy, i.e. the at91. The tx-err value
has just the lower 8bits visible in the register set, but internally its
the full 9bit long. This will result into a wrong tx-err value in case
of a bus-off.
cheers, 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 |
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
