| Message ID | 20200629140442.1043957-18-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix a bunch more W=1 warnings in Misc | expand |
On 2020-06-29 16:04, Lee Jones wrote: > 'fatal_err' is taken as an argument to a static function which is only > invoked once. During this invocation 'fatal_err' is not set. There > doesn't appear to be a good reason to keep it around. > > Also fixes the following W=1 kernel build warning: > > drivers/misc/genwqe/card_base.c:588: warning: Function parameter or > member 'fatal_err' not described in 'genwqe_recover_card' > > Cc: Michael Jung <mijung@gmx.net> > Cc: Michael Ruettger <michael@ibmra.de> > Cc: Frank Haverkamp <haver@linux.ibm.com> > Cc: Joerg-Stephan Vogt <jsvogt@de.ibm.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/misc/genwqe/card_base.c | 18 +++--------------- > 1 file changed, 3 insertions(+), 15 deletions(-) > > diff --git a/drivers/misc/genwqe/card_base.c > b/drivers/misc/genwqe/card_base.c > index bceebf49de2d5..809a6f46f6de3 100644 > --- a/drivers/misc/genwqe/card_base.c > +++ b/drivers/misc/genwqe/card_base.c > @@ -569,30 +569,18 @@ static int genwqe_stop(struct genwqe_dev *cd) > > /** > * genwqe_recover_card() - Try to recover the card if it is possible > - * > - * If fatal_err is set no register access is possible anymore. It is > - * likely that genwqe_start fails in that situation. Proper error > - * handling is required in this case. > + * @cd: GenWQE device information > * > * genwqe_bus_reset() will cause the pci code to call genwqe_remove() > * and later genwqe_probe() for all virtual functions. > */ > -static int genwqe_recover_card(struct genwqe_dev *cd, int fatal_err) > +static int genwqe_recover_card(struct genwqe_dev *cd) > { > int rc; > struct pci_dev *pci_dev = cd->pci_dev; > > genwqe_stop(cd); > > - /* > - * Make sure chip is not reloaded to maintain FFDC. Write SLU > - * Reset Register, CPLDReset field to 0. > - */ > - if (!fatal_err) { > - cd->softreset = 0x70ull; > - __genwqe_writeq(cd, IO_SLC_CFGREG_SOFTRESET, cd->softreset); > - } > - > rc = genwqe_bus_reset(cd); > if (rc != 0) { > dev_err(&pci_dev->dev, > @@ -958,7 +946,7 @@ static int genwqe_health_thread(void *data) > > cd->card_state = GENWQE_CARD_FATAL_ERROR; > > - rc = genwqe_recover_card(cd, 0); > + rc = genwqe_recover_card(cd); > if (rc < 0) { > /* FIXME Card is unusable and needs unbind! */ > goto fatal_error; I think this one I want to keep. Since fatal_err is 0, !fatal_error is 1 and the register write is actually executed. I also want to keep the parameter even though it is only used with 0. The register bit causes a less drastic recovery, but if we would discover that that is not working ok, we rather discard the debug data we get in this case instead of letting the recovery fail. I think it does not hurt to keep it. Maybe you can add a comment? Thanks Frank
On Tue, 30 Jun 2020, haver wrote: > On 2020-06-29 16:04, Lee Jones wrote: > > 'fatal_err' is taken as an argument to a static function which is only > > invoked once. During this invocation 'fatal_err' is not set. There > > doesn't appear to be a good reason to keep it around. > > > > Also fixes the following W=1 kernel build warning: > > > > drivers/misc/genwqe/card_base.c:588: warning: Function parameter or > > member 'fatal_err' not described in 'genwqe_recover_card' > > > > Cc: Michael Jung <mijung@gmx.net> > > Cc: Michael Ruettger <michael@ibmra.de> > > Cc: Frank Haverkamp <haver@linux.ibm.com> > > Cc: Joerg-Stephan Vogt <jsvogt@de.ibm.com> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/misc/genwqe/card_base.c | 18 +++--------------- > > 1 file changed, 3 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/misc/genwqe/card_base.c > > b/drivers/misc/genwqe/card_base.c > > index bceebf49de2d5..809a6f46f6de3 100644 > > --- a/drivers/misc/genwqe/card_base.c > > +++ b/drivers/misc/genwqe/card_base.c > > @@ -569,30 +569,18 @@ static int genwqe_stop(struct genwqe_dev *cd) > > > > /** > > * genwqe_recover_card() - Try to recover the card if it is possible > > - * > > - * If fatal_err is set no register access is possible anymore. It is > > - * likely that genwqe_start fails in that situation. Proper error > > - * handling is required in this case. > > + * @cd: GenWQE device information > > * > > * genwqe_bus_reset() will cause the pci code to call genwqe_remove() > > * and later genwqe_probe() for all virtual functions. > > */ > > -static int genwqe_recover_card(struct genwqe_dev *cd, int fatal_err) > > +static int genwqe_recover_card(struct genwqe_dev *cd) > > { > > int rc; > > struct pci_dev *pci_dev = cd->pci_dev; > > > > genwqe_stop(cd); > > > > - /* > > - * Make sure chip is not reloaded to maintain FFDC. Write SLU > > - * Reset Register, CPLDReset field to 0. > > - */ > > - if (!fatal_err) { > > - cd->softreset = 0x70ull; > > - __genwqe_writeq(cd, IO_SLC_CFGREG_SOFTRESET, cd->softreset); > > - } > > - > > rc = genwqe_bus_reset(cd); > > if (rc != 0) { > > dev_err(&pci_dev->dev, > > @@ -958,7 +946,7 @@ static int genwqe_health_thread(void *data) > > > > cd->card_state = GENWQE_CARD_FATAL_ERROR; > > > > - rc = genwqe_recover_card(cd, 0); > > + rc = genwqe_recover_card(cd); > > if (rc < 0) { > > /* FIXME Card is unusable and needs unbind! */ > > goto fatal_error; > > I think this one I want to keep. Since fatal_err is 0, !fatal_error is 1 and > the register write is actually executed. Ah yes, good catch. What if we *always* did instead then? > I also want to keep the parameter even though it is only used with 0. The > register bit causes a less drastic recovery, but if we would discover that > that is not working ok, we rather discard the debug data we get in this case > instead of letting the recovery fail. I think it does not hurt to keep it. I'm not 100% against it, but it is dead code. > Maybe you can add a comment? If you really want to keep it, I can just update the kerneldoc instead.
On 2020-06-30 11:10, Lee Jones wrote: > On Tue, 30 Jun 2020, haver wrote: > >> On 2020-06-29 16:04, Lee Jones wrote: >> > 'fatal_err' is taken as an argument to a static function which is only >> > invoked once. During this invocation 'fatal_err' is not set. There >> > doesn't appear to be a good reason to keep it around. >> > >> > Also fixes the following W=1 kernel build warning: >> > >> > drivers/misc/genwqe/card_base.c:588: warning: Function parameter or >> > member 'fatal_err' not described in 'genwqe_recover_card' >> > >> > Cc: Michael Jung <mijung@gmx.net> >> > Cc: Michael Ruettger <michael@ibmra.de> >> > Cc: Frank Haverkamp <haver@linux.ibm.com> >> > Cc: Joerg-Stephan Vogt <jsvogt@de.ibm.com> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org> >> > --- >> > drivers/misc/genwqe/card_base.c | 18 +++--------------- >> > 1 file changed, 3 insertions(+), 15 deletions(-) >> > >> > diff --git a/drivers/misc/genwqe/card_base.c >> > b/drivers/misc/genwqe/card_base.c >> > index bceebf49de2d5..809a6f46f6de3 100644 >> > --- a/drivers/misc/genwqe/card_base.c >> > +++ b/drivers/misc/genwqe/card_base.c >> > @@ -569,30 +569,18 @@ static int genwqe_stop(struct genwqe_dev *cd) >> > >> > /** >> > * genwqe_recover_card() - Try to recover the card if it is possible >> > - * >> > - * If fatal_err is set no register access is possible anymore. It is >> > - * likely that genwqe_start fails in that situation. Proper error >> > - * handling is required in this case. >> > + * @cd: GenWQE device information >> > * >> > * genwqe_bus_reset() will cause the pci code to call genwqe_remove() >> > * and later genwqe_probe() for all virtual functions. >> > */ >> > -static int genwqe_recover_card(struct genwqe_dev *cd, int fatal_err) >> > +static int genwqe_recover_card(struct genwqe_dev *cd) >> > { >> > int rc; >> > struct pci_dev *pci_dev = cd->pci_dev; >> > >> > genwqe_stop(cd); >> > >> > - /* >> > - * Make sure chip is not reloaded to maintain FFDC. Write SLU >> > - * Reset Register, CPLDReset field to 0. >> > - */ >> > - if (!fatal_err) { >> > - cd->softreset = 0x70ull; >> > - __genwqe_writeq(cd, IO_SLC_CFGREG_SOFTRESET, cd->softreset); >> > - } >> > - >> > rc = genwqe_bus_reset(cd); >> > if (rc != 0) { >> > dev_err(&pci_dev->dev, >> > @@ -958,7 +946,7 @@ static int genwqe_health_thread(void *data) >> > >> > cd->card_state = GENWQE_CARD_FATAL_ERROR; >> > >> > - rc = genwqe_recover_card(cd, 0); >> > + rc = genwqe_recover_card(cd); >> > if (rc < 0) { >> > /* FIXME Card is unusable and needs unbind! */ >> > goto fatal_error; >> >> I think this one I want to keep. Since fatal_err is 0, !fatal_error is >> 1 and >> the register write is actually executed. > > Ah yes, good catch. > > What if we *always* did instead then? I knew you would ask that ;-). > >> I also want to keep the parameter even though it is only used with 0. >> The >> register bit causes a less drastic recovery, but if we would discover >> that >> that is not working ok, we rather discard the debug data we get in >> this case >> instead of letting the recovery fail. I think it does not hurt to keep >> it. > > I'm not 100% against it, but it is dead code. > >> Maybe you can add a comment? > > If you really want to keep it, I can just update the kerneldoc > instead. I prefer that option. I want to indicate that there are two possible ways to recover on a problem. If we delete the currently not exploited parameter, this information gets lost. Regards Frank
On Tue, 30 Jun 2020, haver wrote: > On 2020-06-30 11:10, Lee Jones wrote: > > On Tue, 30 Jun 2020, haver wrote: > > > > > On 2020-06-29 16:04, Lee Jones wrote: > > > > 'fatal_err' is taken as an argument to a static function which is only > > > > invoked once. During this invocation 'fatal_err' is not set. There > > > > doesn't appear to be a good reason to keep it around. > > > > > > > > Also fixes the following W=1 kernel build warning: > > > > > > > > drivers/misc/genwqe/card_base.c:588: warning: Function parameter or > > > > member 'fatal_err' not described in 'genwqe_recover_card' > > > > > > > > Cc: Michael Jung <mijung@gmx.net> > > > > Cc: Michael Ruettger <michael@ibmra.de> > > > > Cc: Frank Haverkamp <haver@linux.ibm.com> > > > > Cc: Joerg-Stephan Vogt <jsvogt@de.ibm.com> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > --- > > > > drivers/misc/genwqe/card_base.c | 18 +++--------------- > > > > 1 file changed, 3 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/drivers/misc/genwqe/card_base.c > > > > b/drivers/misc/genwqe/card_base.c > > > > index bceebf49de2d5..809a6f46f6de3 100644 > > > > --- a/drivers/misc/genwqe/card_base.c > > > > +++ b/drivers/misc/genwqe/card_base.c > > > > @@ -569,30 +569,18 @@ static int genwqe_stop(struct genwqe_dev *cd) > > > > > > > > /** > > > > * genwqe_recover_card() - Try to recover the card if it is possible > > > > - * > > > > - * If fatal_err is set no register access is possible anymore. It is > > > > - * likely that genwqe_start fails in that situation. Proper error > > > > - * handling is required in this case. > > > > + * @cd: GenWQE device information > > > > * > > > > * genwqe_bus_reset() will cause the pci code to call genwqe_remove() > > > > * and later genwqe_probe() for all virtual functions. > > > > */ > > > > -static int genwqe_recover_card(struct genwqe_dev *cd, int fatal_err) > > > > +static int genwqe_recover_card(struct genwqe_dev *cd) > > > > { > > > > int rc; > > > > struct pci_dev *pci_dev = cd->pci_dev; > > > > > > > > genwqe_stop(cd); > > > > > > > > - /* > > > > - * Make sure chip is not reloaded to maintain FFDC. Write SLU > > > > - * Reset Register, CPLDReset field to 0. > > > > - */ > > > > - if (!fatal_err) { > > > > - cd->softreset = 0x70ull; > > > > - __genwqe_writeq(cd, IO_SLC_CFGREG_SOFTRESET, cd->softreset); > > > > - } > > > > - > > > > rc = genwqe_bus_reset(cd); > > > > if (rc != 0) { > > > > dev_err(&pci_dev->dev, > > > > @@ -958,7 +946,7 @@ static int genwqe_health_thread(void *data) > > > > > > > > cd->card_state = GENWQE_CARD_FATAL_ERROR; > > > > > > > > - rc = genwqe_recover_card(cd, 0); > > > > + rc = genwqe_recover_card(cd); > > > > if (rc < 0) { > > > > /* FIXME Card is unusable and needs unbind! */ > > > > goto fatal_error; > > > > > > I think this one I want to keep. Since fatal_err is 0, !fatal_error > > > is 1 and > > > the register write is actually executed. > > > > Ah yes, good catch. > > > > What if we *always* did instead then? > > I knew you would ask that ;-). > > > > > > I also want to keep the parameter even though it is only used with > > > 0. The > > > register bit causes a less drastic recovery, but if we would > > > discover that > > > that is not working ok, we rather discard the debug data we get in > > > this case > > > instead of letting the recovery fail. I think it does not hurt to > > > keep it. > > > > I'm not 100% against it, but it is dead code. > > > > > Maybe you can add a comment? > > > > If you really want to keep it, I can just update the kerneldoc > > instead. > > I prefer that option. I want to indicate that there are two possible ways to > recover on a problem. If we delete the currently not exploited parameter, > this information gets lost. That's fine. Will fix.
diff --git a/drivers/misc/genwqe/card_base.c b/drivers/misc/genwqe/card_base.c index bceebf49de2d5..809a6f46f6de3 100644 --- a/drivers/misc/genwqe/card_base.c +++ b/drivers/misc/genwqe/card_base.c @@ -569,30 +569,18 @@ static int genwqe_stop(struct genwqe_dev *cd) /** * genwqe_recover_card() - Try to recover the card if it is possible - * - * If fatal_err is set no register access is possible anymore. It is - * likely that genwqe_start fails in that situation. Proper error - * handling is required in this case. + * @cd: GenWQE device information * * genwqe_bus_reset() will cause the pci code to call genwqe_remove() * and later genwqe_probe() for all virtual functions. */ -static int genwqe_recover_card(struct genwqe_dev *cd, int fatal_err) +static int genwqe_recover_card(struct genwqe_dev *cd) { int rc; struct pci_dev *pci_dev = cd->pci_dev; genwqe_stop(cd); - /* - * Make sure chip is not reloaded to maintain FFDC. Write SLU - * Reset Register, CPLDReset field to 0. - */ - if (!fatal_err) { - cd->softreset = 0x70ull; - __genwqe_writeq(cd, IO_SLC_CFGREG_SOFTRESET, cd->softreset); - } - rc = genwqe_bus_reset(cd); if (rc != 0) { dev_err(&pci_dev->dev, @@ -958,7 +946,7 @@ static int genwqe_health_thread(void *data) cd->card_state = GENWQE_CARD_FATAL_ERROR; - rc = genwqe_recover_card(cd, 0); + rc = genwqe_recover_card(cd); if (rc < 0) { /* FIXME Card is unusable and needs unbind! */ goto fatal_error;
'fatal_err' is taken as an argument to a static function which is only invoked once. During this invocation 'fatal_err' is not set. There doesn't appear to be a good reason to keep it around. Also fixes the following W=1 kernel build warning: drivers/misc/genwqe/card_base.c:588: warning: Function parameter or member 'fatal_err' not described in 'genwqe_recover_card' Cc: Michael Jung <mijung@gmx.net> Cc: Michael Ruettger <michael@ibmra.de> Cc: Frank Haverkamp <haver@linux.ibm.com> Cc: Joerg-Stephan Vogt <jsvogt@de.ibm.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/misc/genwqe/card_base.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)