linux-kernel - Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller

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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 18 Jul 2013 14:56:07 +0300
From:	Felipe Balbi <balbi@...com>
To:	Sourav Poddar <sourav.poddar@...com>
CC:	Mark Brown <broonie@...nel.org>,
	<spi-devel-general@...ts.sourceforge.net>,
	<grant.likely@...aro.org>, <balbi@...com>, <rnayak@...com>,
	<linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller

Hi,

On Thu, Jul 18, 2013 at 05:15:45PM +0530, Sourav Poddar wrote:
> >>+	list_for_each_entry(t,&m->transfers, transfer_list) {
> >>+		qspi->cmd |= QSPI_WLEN(t->bits_per_word);
> >>+		qspi->cmd |= QSPI_WC_CMD_INT_EN;
> >>+
> >>+		ret = qspi_transfer_msg(qspi, t);
> >>+		if (ret) {
> >>+			dev_dbg(qspi->dev, "transfer message failed\n");
> >>+			return -EINVAL;
> >>+		}
> >>+
> >>+		m->actual_length += t->len;
> >>+
> >>+		if (list_is_last(&t->transfer_list,&m->transfers))
> >>+			goto out;
> >>+	}
> >The use of list_is_last() here is *realy* strange - what's going on
> >there?
> >
> We are checking if there is any transfer left, if no we are signalling the
> flash device about the end of transfer.

I'll quote your diff below because I wanted to show where your 'out'
label lies:

+       list_for_each_entry(t, &m->transfers, transfer_list) {
+               qspi->cmd |= QSPI_WLEN(t->bits_per_word);
+               qspi->cmd |= QSPI_WC_CMD_INT_EN;
+
+               ret = qspi_transfer_msg(qspi, t);
+               if (ret) {
+                       dev_dbg(qspi->dev, "transfer message failed\n");
+                       return -EINVAL;
+               }
+
+               m->actual_length += t->len;
+
+               if (list_is_last(&t->transfer_list, &m->transfers))
+                       goto out;
+       }
+
+out:
+       m->status = status;
+       spi_finalize_current_message(master);

see how it's place right after the closing curly brackets ? In that
case, why do you need it ? list_for_each_entry() will do exactly what it
says: iterate over each entry on the entry. When it reaches the last
one, the loop will end.

This renders your list_is_last() check useless. In fact, you could've
figured that out if you just spent the time to read
list_for_each_entry() carefully.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)