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.
> +
> + 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);
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
