On Mon, May 17, 2021 at 07:38:19PM +0200, Stefan Wahren wrote:
> 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.
It should be get/put or hold/release.
>
> >
> >> +
> >> + 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.
>
Nah, don't worry about it. You copied that WARN_ON() from the original
code so don't change it except in a different patch.
regards,
dan carpenter
