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.
>
> 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.
Anyway, it's not related to the state change handling ;-).
>>>> 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".
Why, the calling function needs to allocate the frame anyway. Filling
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].
> 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.
>> 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.
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?
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
