| Message ID | 5f1b69cd3a52e367f9f5014a3613768c8634408c.1561997809.git.alifm@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some vfio-ccw fixes | expand |
On Mon, 1 Jul 2019 12:23:44 -0400 Farhan Ali <alifm@linux.ibm.com> wrote: > We don't set cp->initialized to true so calling cp_free > will just return and not do anything. > > Signed-off-by: Farhan Ali <alifm@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c > index 5ac4c1e..cab1be9 100644 > --- a/drivers/s390/cio/vfio_ccw_cp.c > +++ b/drivers/s390/cio/vfio_ccw_cp.c > @@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) > > /* Build a ccwchain for the first CCW segment */ > ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); > - if (ret) > - cp_free(cp); Makes sense; hopefully ccwchain_handle_ccw() cleans up correctly on error :) (I think it does) Maybe add a comment /* ccwchain_handle_ccw() already cleans up on error */ so we don't stumble over this in the future? (Also, does this want a Fixes: tag?) > > if (!ret) > cp->initialized = true;
On 07/02/2019 04:42 AM, Cornelia Huck wrote: > On Mon, 1 Jul 2019 12:23:44 -0400 > Farhan Ali <alifm@linux.ibm.com> wrote: > >> We don't set cp->initialized to true so calling cp_free >> will just return and not do anything. >> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com> >> --- >> drivers/s390/cio/vfio_ccw_cp.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c >> index 5ac4c1e..cab1be9 100644 >> --- a/drivers/s390/cio/vfio_ccw_cp.c >> +++ b/drivers/s390/cio/vfio_ccw_cp.c >> @@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) >> >> /* Build a ccwchain for the first CCW segment */ >> ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); >> - if (ret) >> - cp_free(cp); > > Makes sense; hopefully ccwchain_handle_ccw() cleans up correctly on > error :) (I think it does) > I have checked that it does as well, but wouldn't hurt if someone else also glances over once again :) > Maybe add a comment > > /* ccwchain_handle_ccw() already cleans up on error */ > > so we don't stumble over this in the future? Sure. > > (Also, does this want a Fixes: tag?) This might warrant a fixes tag as well. > >> >> if (!ret) >> cp->initialized = true; > >
On 7/2/19 9:58 AM, Farhan Ali wrote: > > > On 07/02/2019 04:42 AM, Cornelia Huck wrote: >> On Mon, 1 Jul 2019 12:23:44 -0400 >> Farhan Ali <alifm@linux.ibm.com> wrote: >> >>> We don't set cp->initialized to true so calling cp_free >>> will just return and not do anything. >>> >>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com> >>> --- >>> drivers/s390/cio/vfio_ccw_cp.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c >>> b/drivers/s390/cio/vfio_ccw_cp.c >>> index 5ac4c1e..cab1be9 100644 >>> --- a/drivers/s390/cio/vfio_ccw_cp.c >>> +++ b/drivers/s390/cio/vfio_ccw_cp.c >>> @@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct >>> device *mdev, union orb *orb) >>> /* Build a ccwchain for the first CCW segment */ >>> ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); >>> - if (ret) >>> - cp_free(cp); >> >> Makes sense; hopefully ccwchain_handle_ccw() cleans up correctly on >> error :) (I think it does) >> > > I have checked that it does as well, but wouldn't hurt if someone else > also glances over once again :) Oh noes. What happens once we start encountering TICs? If we do: ccwchain_handle_ccw() (OK) ccwchain_loop_tic() (OK) ccwchain_handle_ccw() (FAIL) The first _handle_ccw() will have added a ccwchain to the cp list, which doesn't appear to get cleaned up now. That used to be done in cp_init() until I squashed cp_free and cp_unpin_free. :( > >> Maybe add a comment >> >> /* ccwchain_handle_ccw() already cleans up on error */ >> >> so we don't stumble over this in the future? > > Sure. > >> >> (Also, does this want a Fixes: tag?) > > This might warrant a fixes tag as well. >> >>> if (!ret) >>> cp->initialized = true; >> >>
On 07/02/2019 12:15 PM, Eric Farman wrote: > > > On 7/2/19 9:58 AM, Farhan Ali wrote: >> >> >> On 07/02/2019 04:42 AM, Cornelia Huck wrote: >>> On Mon, 1 Jul 2019 12:23:44 -0400 >>> Farhan Ali <alifm@linux.ibm.com> wrote: >>> >>>> We don't set cp->initialized to true so calling cp_free >>>> will just return and not do anything. >>>> >>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com> >>>> --- >>>> drivers/s390/cio/vfio_ccw_cp.c | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c >>>> b/drivers/s390/cio/vfio_ccw_cp.c >>>> index 5ac4c1e..cab1be9 100644 >>>> --- a/drivers/s390/cio/vfio_ccw_cp.c >>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c >>>> @@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct >>>> device *mdev, union orb *orb) >>>> /* Build a ccwchain for the first CCW segment */ >>>> ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); >>>> - if (ret) >>>> - cp_free(cp); >>> >>> Makes sense; hopefully ccwchain_handle_ccw() cleans up correctly on >>> error :) (I think it does) >>> >> >> I have checked that it does as well, but wouldn't hurt if someone else >> also glances over once again :) > > Oh noes. What happens once we start encountering TICs? If we do: > > ccwchain_handle_ccw() (OK) > ccwchain_loop_tic() (OK) > ccwchain_handle_ccw() (FAIL) > > The first _handle_ccw() will have added a ccwchain to the cp list, which > doesn't appear to get cleaned up now. That used to be done in cp_init() > until I squashed cp_free and cp_unpin_free. :( Yup, you are right we are not freeing the chain correctly. Will fix it in v2. > >> >>> Maybe add a comment >>> >>> /* ccwchain_handle_ccw() already cleans up on error */ >>> >>> so we don't stumble over this in the future? >> >> Sure. >> >>> >>> (Also, does this want a Fixes: tag?) >> >> This might warrant a fixes tag as well. >>> >>>> if (!ret) >>>> cp->initialized = true; >>> >>> >
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index 5ac4c1e..cab1be9 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) /* Build a ccwchain for the first CCW segment */ ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); - if (ret) - cp_free(cp); if (!ret) cp->initialized = true;
We don't set cp->initialized to true so calling cp_free will just return and not do anything. Signed-off-by: Farhan Ali <alifm@linux.ibm.com> --- drivers/s390/cio/vfio_ccw_cp.c | 2 -- 1 file changed, 2 deletions(-)