Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 1 Mar 2021 00:04:30 +0000
From:   "Life is hard, and then you die" <ronald@innovation.ch>
To:     Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc:     Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        linux-iio@vger.kernel.org
Subject: Re: [PATCH 4/5] HID: apple-ibridge: Add Apple iBridge HID driver for
 T1 chip.
Message-ID: <20210301000430.GA754582@innovation.ch>
References: <20210228012643.69944-1-ronald@innovation.ch>
 <20210228012643.69944-5-ronald@innovation.ch>
 <20210228150239.00007d34@Huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
In-Reply-To: <20210228150239.00007d34@Huawei.com>
Precedence: bulk


  Hi Jonathan,

On Sun, Feb 28, 2021 at 03:02:39PM +0000, Jonathan Cameron wrote:
> On Sat, 27 Feb 2021 17:26:42 -0800
> Ronald Tschal?r <ronald@innovation.ch> wrote:
[snip]
> > +#ifdef CONFIG_PM
> > +/**
> > + * appleib_forward_int_op() - Forward a hid-driver callback to all drivers on
> > + * all virtual HID devices attached to the given real HID device.
> > + * @hdev the real hid-device
> > + * @forward a function that calls the callback on the given driver
> > + * @args arguments for the forward function
> > + *
> > + * This is for callbacks that return a status as an int.
> > + *
> > + * Returns: 0 on success, or the first error returned by the @forward function.
> > + */
> > +static int appleib_forward_int_op(struct hid_device *hdev,
> > +				  int (*forward)(struct hid_driver *,
> > +						 struct hid_device *, void *),
> > +				  void *args)
> > +{
> > +	struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
> > +	struct hid_device *sub_hdev;
> > +	int rc = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> > +		sub_hdev = hdev_info->sub_hdevs[i];
> > +		if (sub_hdev->driver) {
> > +			rc = forward(sub_hdev->driver, sub_hdev, args);
> > +			if (rc)
> > +				break;
> 
> return rc; here would be cleaner.
> 
> > +		}
> > +
> > +		break;
> 
> This is unusual.  It's a for loop but as far as I can see only first iteration
> can ever run as we exit the loop at this break if we haven't done so earlier.
> What is the intent here?
> 
> > +	}
> > +
> > +	return rc;
> > +}

Ho boy, good catch! This is simply a mistake. As you say, it should
(and does now) read:

	for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
		sub_hdev = hdev_info->sub_hdevs[i];
		if (sub_hdev->driver) {
			rc = forward(sub_hdev->driver, sub_hdev, args);
			if (rc)
				return rc;
		}
	}

	return rc;

Thanks.


  Cheers,

  Ronald