Re: [PATCH 17/20] misc: genwqe: card_base: Do not pass unused argument 'fatal_err'
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
> > > > Cc: Michael Ruettger
> > > > Cc: Frank Haverkamp
> > > > Cc: Joerg-Stephan Vogt
> > > > Signed-off-by: Lee Jones
> > > > ---
> > > > 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.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 17/20] misc: genwqe: card_base: Do not pass unused argument 'fatal_err'
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
> Cc: Michael Ruettger
> Cc: Frank Haverkamp
> Cc: Joerg-Stephan Vogt
> Signed-off-by: Lee Jones
> ---
> 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
Re: [PATCH 17/20] misc: genwqe: card_base: Do not pass unused argument 'fatal_err'
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
> > Cc: Michael Ruettger
> > Cc: Frank Haverkamp
> > Cc: Joerg-Stephan Vogt
> > Signed-off-by: Lee Jones
> > ---
> > 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.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 17/20] misc: genwqe: card_base: Do not pass unused argument 'fatal_err'
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
Cc: Michael Ruettger
Cc: Frank Haverkamp
Cc: Joerg-Stephan Vogt
Signed-off-by: Lee Jones
---
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
