linux-kernel - Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2f4e2139-3570-403c-aac1-c31c2b21b014@stanley.mountain>
Date: Sun, 13 Oct 2024 15:14:11 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Umang Jain <umang.jain@...asonboard.com>
Cc: Kieran Bingham <kieran.bingham@...asonboard.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-rpi-kernel@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	kernel-list@...pberrypi.com, Stefan Wahren <wahrenst@....net>
Subject: Re: [PATCH v3 4/6] staging: vchiq_core: Refactor notify_bulks()

On Sun, Oct 13, 2024 at 01:03:45PM +0530, Umang Jain wrote:
> > > +               spin_unlock(&service->state->bulk_waiter_spinlock);
> > > +
> > > +               status = 0;
> > This just looks odd here. If it weren't for this I'd have probably been
> > fine with the initialisation of status
> > 
> > > +       } else if (bulk->mode == VCHIQ_BULK_MODE_CALLBACK) {
> > > +               enum vchiq_reason reason = get_bulk_reason(bulk);
> > > +               status = make_service_callback(service, reason, NULL,
> > > +                                              bulk->userdata);
> > I think I would probably just drop the int status altogether and make this
> > 
> > 		return make_service_callback(service, reason, NULL,
> > 					     bulk->userdata);
> > 
> > > +       }
> > > +
> > > +       return status;
> > And return 0 here. Then we get rid of the awkward initialisation and
> > usages above.
> 
> I usually have the tendency to minimise return  statements in a routine and
> ideally target for single return statement at the end.

I feel like the "one return per function" style rule is an anti-pattern.  I
feel like it's better to handle errors right away.  Then the code which is
indented one tab is the success path and the code which is indented more is
the edge cases.

> 
>  But I do agree on the awkward initialisation of status = 0

I sent my email and then I thought.  Actually the solution here is to do:

		status = make_service_callback(service, reason, NULL,
					       bulk->userdata);
		if (status)
			return status;
	}

	return 0;

This handles the error right away and avoids mixing the error paths with the
success paths.  Plus I like a big "return 0;" at the end of my functions.

I like Kieran's approach as well.

But, I see now that I have misread the function.  I'm not sure what is the most
readable way to write it.  Maybe:

	int status = 0;

	if (bulk->mode == VCHIQ_BULK_MODE_BLOCKING) {
		...
	} else if (bulk->mode == VCHIQ_BULK_MODE_CALLBACK) {
		...
		status = make_service_callback();
	} else {
		status = -EINVAL;
	}

	return status;

Probably whatever you decide is fine.  You care more about this code than I do
for sure.

regards,
dan carpenter