Hi Dan,
Am 17.05.21 um 13:49 schrieb Dan Carpenter:
> On Sat, May 15, 2021 at 09:10:57PM +0200, Stefan Wahren wrote:
>> +bail_not_ready:
>> + if (service)
>> + unlock_service(service);
> Not related to this patch, but the unlock_service() function doesn't
> having anything to do with locking so it's confusing.
Any preference about the naming of the both functions (lock_service /
unlock_service)? At the end they are just wrapper around kref_get /
kref_put.
>
>> +
>> + return ret;
>> +}
>> +
>> +/* Called by the slot handler thread */
>> +static void
>> +parse_rx_slots(struct vchiq_state *state)
>> +{
>> + struct vchiq_shared_state *remote = state->remote;
>> + int tx_pos;
>> +
>> + DEBUG_INITIALISE(state->local)
>> +
>> + tx_pos = remote->tx_pos;
>> +
>> + while (state->rx_pos != tx_pos) {
>> + struct vchiq_header *header;
>> + int size;
>> +
>> + DEBUG_TRACE(PARSE_LINE);
>> + if (!state->rx_data) {
>> + int rx_index;
>> +
>> + WARN_ON(!((state->rx_pos & VCHIQ_SLOT_MASK) == 0));
> Also not related but this WARN_ON() has a confusing double negative. In
> future patches someone could change this to:
>
> WARN_ON(state->rx_pos & VCHIQ_SLOT_MASK);
I could add this to V2 of this series.
Regards
Stefan
>
> Back in the day, I sort of hired someone to create a TODO website so
> that if you're reviewing comments you could put a note:
>
> TODO: staging: vchiq_core: tidy up naming
>
> Then the website would make a list of all the TODOs for that driver.
> But then I never went live with the website and we introduces
> lore.kernel.org and it's almost searchable so hopefully some day it will
> be able to do searches like this...
>
> regards,
> dan carpenter
>
