Please review patch for aic7xxx_pci.c

[Ulrich Spörlein]

[adding back scsi at freebsd.org]

On Tue, 2013-07-23 at 22:45:30 +0200, Ulrich Spörlein wrote:
> Thanks, but maybe it's better to have the discussion here.
> 
> You wrote:
> The difference in the argument order between the declaration and definition is
> likely what led to this programming error. Your patch should also correct the
> argument reversal in the declaration of ahc_9005_subdevinfo_valid().
> 
> The argument order you've used seems the best choice since it matches what is
> used in ahc_compose_id().
> 
> 
> 
> I'm not sure that reversing the arguments in the function definition is
> the right way. This would make the patch a no-op.
> 
> Consider the flow pre r95378
> 
> if (ahc_get_pci_function(pci) > 0
>   && subvendor == 0x9005
>   && subdevice != device
>   && SUBID_9005_TYPE_KNOWN(subdevice) != 0
>   && SUBID_9005_MFUNCENB(subdevice) == 0)
>          return (NULL);
> 
> and after r95378
> 
> if (ahc_get_pci_function(pci) > 0
>   && ahc_9005_subdevinfo_valid(vendor, device, subvendor, subdevice)
>    -> which translates to
>         if (device == 0x9005
>           && subdevice == 0x9005
>           && subvendor != vendor
>           && SUBID_9005_TYPE_KNOWN(subvendor) != 0
>           && SUBID_9005_TYPE(subvendor)
>           && DEVID_9005_TYPE(vendor) == DEVID_9005_TYPE_HBA
>   && SUBID_9005_MFUNCENB(subdevice) == 0)
>          return (NULL);
> 
> But this is nonsensical. 0x9005 is the vendor ID for adaptec,
> DEVID_9005_TYPE() should test the device id, etc.
> 
> 
> The strangest part is that, although this function tests for "validity",
> when it returns 1 or true, then ahc_find_pci_device() is more likely to
> return NULL. This all seems horribly backwards.
> 
> It's unlikely that someone will want to run -CURRENT on a machine with
> that hardware in it, so maybe we should axe it?
> 
> Cheers,
> Uli
> 
> On Tue, 2013-07-23 at 13:34:02 -0600, Justin T. Gibbs wrote:
> > Done.
> > 
> > --
> > Justin
> > 
> > On Jul 23, 2013, at 1:11 AM, Ulrich Spörlein <uqs at freebsd.org> wrote:
> > 
> > > [adding scsi, it would be nice if I could get another set of eyes on
> > > this as I don't have the hardware to test this]
> > > 
> > > On Tue, 2013-07-16 at 11:49:27 +0200, Ulrich Spörlein wrote:
> > >> 
> > >> Hey Justin, Ken,
> > >> 
> > >> Coverity found one more instance of swapped parameters in the kernel,
> > >> introduced quite some time ago. It's probably hard to get the hardware
> > >> to test this change these days ...
> > >> 
> > >> Please see
> > >> https://github.com/uqs/freebsd-head/commit/2f8f438a380c2a52a2e9f266cd716f56c8a4bb75
> > >> and leave comments, or reply to this mail.
> > >> 
> > >> You can see the diff that introduced the problem here:
> > >> http://git.freebsd.your.org/gitweb/?p=freebsd.git;a=blobdiff;f=sys/dev/aic7xxx/aic7xxx_pci.c;h=42dcdcc57f09d11199aabd6b224f2dad730f0733;hp=a4c0f4672cab3eb965d7acf1a580162168ca9716;hb=27ca4db2579f6e74861db16299eeb52e158fa7a7;hpb=06842004edacee7beec6cb72239ded59709506a2
> > >> 
> > >> or here:
> > >> 
> > >> https://github.com/uqs/freebsd-head/commit/9696016b0572574bb656fb109ff2916127fa8cb8
> > >> 
> > >> Thanks!
> > >> Uli
> > > _______________________________________________
> > > freebsd-scsi at freebsd.org mailing list
> > > http://lists.freebsd.org/mailman/listinfo/freebsd-scsi
> > > To unsubscribe, send any mail to "freebsd-scsi-unsubscribe at freebsd.org"
> >