LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Peter Teoh" <htmldeveloper@gmail.com>
To: "Adrian Bunk" <bunk@kernel.org>
Cc: "Bartlomiej Zolnierkiewicz" <bzolnier@gmail.com>,
	linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: ide_register_hw(): buggy code
Date: Tue, 4 Mar 2008 00:03:00 +0800	[thread overview]
Message-ID: <804dabb00803030803v1dbbb33fh6779b8c4d072a908@mail.gmail.com> (raw)
In-Reply-To: <20080302151924.GJ25835@cs181133002.pp.htv.fi>

On Sun, Mar 2, 2008 at 11:19 PM, Adrian Bunk <bunk@kernel.org> wrote:
> The Coverity checker spotted the following bogus change to
>  ide_register_hw() in commit 9e016a719209d95338e314b46c3012cc7feaaeec:
>
>  <--  snip  -->
>
>  ...
>  +               hwif = ide_deprecated_find_port(hw->io_ports[IDE_DATA_OFFSET]);
>  +               index = hwif->index;
>  +               if (hwif)
>  +                       goto found;
>                 for (index = 0; index < MAX_HWIFS; index++)
>  ...
>
>  <--  snip  -->
>
>  It's impossible to reach the for() loop without Oopsing before.
>
>  Can you either fix this for 2.6.25 or push your patch that removes
>  ide_register_hw() for 2.6.25?
>

My question is:

a.   why is "retry=1", and then the do while loop always end up the
loop being one round executed only?   Why not just remove the while
loop entirely?

b.   not sure if your statement above implied this, but checking for
hwif!=0 should be before index.  ???

c.   "index = hwif->index;" should not be there, but after "found".
Is that correct?

int ide_register_hw(hw_regs_t *hw, void (*quirkproc)(ide_drive_t *),
                    ide_hwif_t **hwifp)
{
        int index, retry = 1;
        ide_hwif_t *hwif;
        u8 idx[4] = { 0xff, 0xff, 0xff, 0xff };

        do {
                hwif = ide_deprecated_find_port(hw->io_ports[IDE_DATA_OFFSET]);
                index = hwif->index;
                if (hwif)
                        goto found;
                for (index = 0; index < MAX_HWIFS; index++)
                        ide_unregister(index, 1, 1);
        } while (retry--);
        return -1;
found:
        if (hwif->present)


The patch I am thinking goes something like this:

--- ide/ide.c   2008-03-03 20:10:28.000000000 +0800
+++ ide/ide_new.c       2008-03-04 00:09:46.000000000 +0800
@@ -661,20 +661,18 @@ EXPORT_SYMBOL_GPL(ide_deprecated_find_po
 int ide_register_hw(hw_regs_t *hw, void (*quirkproc)(ide_drive_t *),
                    ide_hwif_t **hwifp)
 {
-       int index, retry = 1;
+       int index;
        ide_hwif_t *hwif;
        u8 idx[4] = { 0xff, 0xff, 0xff, 0xff };

-       do {
-               hwif = ide_deprecated_find_port(hw->io_ports[IDE_DATA_OFFSET]);
-               index = hwif->index;
-               if (hwif)
-                       goto found;
-               for (index = 0; index < MAX_HWIFS; index++)
-                       ide_unregister(index, 1, 1);
-       } while (retry--);
+       hwif = ide_deprecated_find_port(hw->io_ports[IDE_DATA_OFFSET]);
+       if (hwif)
+               goto found;
+       for (index = 0; index < MAX_HWIFS; index++)
+               ide_unregister(index, 1, 1);
        return -1;
 found:
+       index = hwif->index;
        if (hwif->present)
                ide_unregister(index, 0, 1);
        else if (!hwif->hold)

Please comment.

-- 
Regards,
Peter Teoh

  reply	other threads:[~2008-03-03 16:03 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-03-02 15:19 Adrian Bunk
2008-03-03 16:03 ` Peter Teoh [this message]
2008-03-03 22:29   ` Bartlomiej Zolnierkiewicz
2008-03-04  1:01     ` Peter Teoh
2008-03-04 20:57       ` Bartlomiej Zolnierkiewicz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=804dabb00803030803v1dbbb33fh6779b8c4d072a908@mail.gmail.com \
    --to=htmldeveloper@gmail.com \
    --cc=bunk@kernel.org \
    --cc=bzolnier@gmail.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: ide_register_hw(): buggy code' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).