Re: git: d836c48e7110 - main - cam_periph: wired is really a bool, update it to a bool.

On Mon, Nov 29, 2021 at 6:21 AM Mateusz Guzik <mjguzik@gmail.com> wrote:

> On 11/5/21, Warner Losh <imp@freebsd.org> wrote:
> > The branch main has been updated by imp:
> >
> > URL:
> >
> https://cgit.FreeBSD.org/src/commit/?id=d836c48e7110f2894885cf84ce8990f7916663cc
> >
> > commit d836c48e7110f2894885cf84ce8990f7916663cc
> > Author:     Warner Losh <imp@FreeBSD.org>
> > AuthorDate: 2021-11-05 14:56:48 +0000
> > Commit:     Warner Losh <imp@FreeBSD.org>
> > CommitDate: 2021-11-05 14:56:48 +0000
> >
> >     cam_periph: wired is really a bool, update it to a bool.
> >
> >     Sponsored by:           Netflix
> >     Reviewed by:            scottl
> >     Differential Revision:  https://reviews.freebsd.org/D32823
> > ---
> >  sys/cam/cam_periph.c | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/sys/cam/cam_periph.c b/sys/cam/cam_periph.c
> > index bb4baaf0888f..54fe9a0ef40c 100644
> > --- a/sys/cam/cam_periph.c
> > +++ b/sys/cam/cam_periph.c
>
> > @@ -600,8 +600,9 @@ static u_int
> >  camperiphunit(struct periph_driver *p_drv, path_id_t pathid,
> >      target_id_t target, lun_id_t lun, const char *sn)
> >  {
> > +     bool    wired;
> >       u_int   unit;
> > -     int     wired, i, val, dunit;
> > +     int     i, val, dunit;
> >       const char *dname, *strval;
> >       char    pathbuf[32], *periph_name;
> >
> > @@ -610,29 +611,29 @@ camperiphunit(struct periph_driver *p_drv,
> path_id_t
> > pathid,
> >       unit = 0;
> >       i = 0;
> >       dname = periph_name;
> > -     for (wired = 0; resource_find_dev(&i, dname, &dunit, NULL, NULL)
> == 0;
> > -          wired = 0) {
> > +     while (resource_find_dev(&i, dname, &dunit, NULL, NULL) == 0) {
> > +             wired = false;
>
> This has a side effect of no longer initializing wired if the first
> resource_find_dev call returns != 0, which in turn causes KMSAN to
> panic due to:
>
>         unit = camperiphnextunit(p_drv, unit, wired, pathid, target, lun);
>
> below.
>
> It is unclear to me what this code should do. Plopping 'wired =
> false;' upfront at least restores the previous state and prevents the
> panic.
>

https://reviews.freebsd.org/D33163

I made this change at the last minute due to a review comment to change
this to a while loop. wired should always be false unless we find something
to the contrary. Plus, the old code did that as well. Taken together, the
change
is obvious. I never saw it on my systems because all of them wire units in
different ways.


> >               if (resource_string_value(dname, dunit, "at", &strval) ==
> 0) {
> >                       if (strcmp(strval, pathbuf) != 0)
> >                               continue;
> > -                     wired++;
> > +                     wired = true;
> >               }
> >               if (resource_int_value(dname, dunit, "target", &val) == 0)
> {
> >                       if (val != target)
> >                               continue;
> > -                     wired++;
> > +                     wired = true;
> >               }
> >               if (resource_int_value(dname, dunit, "lun", &val) == 0) {
> >                       if (val != lun)
> >                               continue;
> > -                     wired++;
> > +                     wired = true;
> >               }
> >               if (resource_string_value(dname, dunit, "sn", &strval) ==
> 0) {
> >                       if (sn == NULL || strcmp(strval, sn) != 0)
> >                               continue;
> > -                     wired++;
> > +                     wired = true;
> >               }
> > -             if (wired != 0) {
> > +             if (wired) {
> >                       unit = dunit;
> >                       break;
> >               }
> >
>
>
> --
> Mateusz Guzik <mjguzik gmail.com>
>