Re: git: d836c48e7110 - main - cam_periph: wired is really a bool, update it to a bool.
Date: Mon, 29 Nov 2021 15:14:37 UTC
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> >