Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Xiaomeng Tong <xiam0nd.tong@gmail.com>
To:     jakobkoschel@gmail.com
Cc:     akpm@osdl.org, hverkuil-cisco@xs4all.nl,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
        mchehab@kernel.org, v4l@cerqueira.org, xiam0nd.tong@gmail.com,
        yangyingliang@huawei.com
Subject: Re: [PATCH] saa7134: fix incorrect check to determine if no element is found in list
Date:   Sun, 20 Mar 2022 17:14:38 +0800
Message-Id: <20220320091438.16329-1-xiam0nd.tong@gmail.com>
In-Reply-To: <86AD565C-ADD2-4000-BF99-487EF9FCDB2F@gmail.com>
References: <86AD565C-ADD2-4000-BF99-487EF9FCDB2F@gmail.com>
Precedence: bulk

On Sun, 20 Mar 2022 07:12:33 +0100,
Jakob Koschel <jakobkoschel@gmail.com> wrote:
> > On 20. Mar 2022, at 03:57, Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
> > 
> > The bug is here: "if (dev == NULL)".
> > 
> > The list iterator value will *always* be set and non-NULL by
> > list_for_each_entry(), so it is incorrect to assume that the iterator
> > value will be NULL if the list is empty or no element is found in list.
> > 
> > Use a new value 'iter' as the list iterator, while use the old value
> > 'req' as a dedicated pointer to point to the found element, which
> > 1. can fix this bug, due to now 'req' is NULL only if it's not found.
> > 2. do not need to change all the uses of 'req' after the loop.
> > 3. can also limit the scope of the list iterator 'iter' *only inside*
> >   the traversal loop by simply declaring 'iter' inside the loop in the
> >   future, as usage of the iterator outside of the list_for_each_entry
> >   is considered harmful. https://lkml.org/lkml/2022/2/17/1032
> > 
> > Fixes: 4aabf6331f89c ("[PATCH] v4l: (951) Make saa7134-oss as a stand-alone module")
> > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> > ---
> > drivers/media/pci/saa7134/saa7134-alsa.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/pci/saa7134/saa7134-alsa.c b/drivers/media/pci/saa7134/saa7134-alsa.c
> > index fb24d2ed3621..4955f7e7c5bf 100644
> > --- a/drivers/media/pci/saa7134/saa7134-alsa.c
> > +++ b/drivers/media/pci/saa7134/saa7134-alsa.c
> > @@ -1215,18 +1215,21 @@ static int alsa_device_exit(struct saa7134_dev *dev)
> > static int saa7134_alsa_init(void)
> > {
> > 	struct saa7134_dev *dev = NULL;
> > +	struct saa7134_dev *iter;
> > 
> > 	saa7134_dmasound_init = alsa_device_init;
> > 	saa7134_dmasound_exit = alsa_device_exit;
> > 
> > 	pr_info("saa7134 ALSA driver for DMA sound loaded\n");
> > 
> > -	list_for_each_entry(dev, &saa7134_devlist, devlist) {
> > -		if (dev->pci->device == PCI_DEVICE_ID_PHILIPS_SAA7130)
> > +	list_for_each_entry(iter, &saa7134_devlist, devlist) {
> > +		dev = iter;
> > +
> > +		if (iter->pci->device == PCI_DEVICE_ID_PHILIPS_SAA7130)
> > 			pr_info("%s/alsa: %s doesn't support digital audio\n",
> > -				dev->name, saa7134_boards[dev->board].name);
> > +				iter->name, saa7134_boards[iter->board].name);
> > 		else
> > -			alsa_device_init(dev);
> > +			alsa_device_init(iter);
> > 	}
> > 
> > 	if (dev == NULL)
> 
> I could be wrong, but judging from the printed message "saa7134 ALSA: no saa7134 cards found"
> and how the code is structured, I got the impression that the intention for this code was
> to only execute if the list is empty. 
> 
> There is no break or anything so it's not looking for a specific element.
> 
> It might make more sense to simply replace this check with:
> 	if (list_empty(&saa7134_devlist))

Oh, yes. I think you are right. In addtion, the unneeded initializer for
'dev' can also be removed because the list iterator is always initialized.

Thank you for the suggestion!

--
Xiaomeng Tong