Replace function names in message strings with __func__ to fix
all checkpatch warnings like:
WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
this function's name, in a string
Signed-off-by: Soumya Negi <[email protected]>
---
drivers/staging/vme_user/vme.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
index e8c2c1e77b7d..11c1df12b657 100644
--- a/drivers/staging/vme_user/vme.c
+++ b/drivers/staging/vme_user/vme.c
@@ -422,7 +422,7 @@ int vme_slave_get(struct vme_resource *resource, int *enabled,
image = list_entry(resource->entry, struct vme_slave_resource, list);
if (!bridge->slave_get) {
- dev_err(bridge->parent, "vme_slave_get not supported\n");
+ dev_err(bridge->parent, "%s not supported\n", __func__);
return -EINVAL;
}
@@ -572,7 +572,7 @@ int vme_master_set(struct vme_resource *resource, int enabled,
image = list_entry(resource->entry, struct vme_master_resource, list);
if (!bridge->master_set) {
- dev_warn(bridge->parent, "vme_master_set not supported\n");
+ dev_warn(bridge->parent, "%s not supported\n", __func__);
return -EINVAL;
}
@@ -1565,7 +1565,7 @@ int vme_lm_set(struct vme_resource *resource, unsigned long long lm_base,
lm = list_entry(resource->entry, struct vme_lm_resource, list);
if (!bridge->lm_set) {
- dev_err(bridge->parent, "vme_lm_set not supported\n");
+ dev_err(bridge->parent, "%s not supported\n", __func__);
return -EINVAL;
}
@@ -1601,7 +1601,7 @@ int vme_lm_get(struct vme_resource *resource, unsigned long long *lm_base,
lm = list_entry(resource->entry, struct vme_lm_resource, list);
if (!bridge->lm_get) {
- dev_err(bridge->parent, "vme_lm_get not supported\n");
+ dev_err(bridge->parent, "%s not supported\n", __func__);
return -EINVAL;
}
@@ -1638,7 +1638,7 @@ int vme_lm_attach(struct vme_resource *resource, int monitor,
lm = list_entry(resource->entry, struct vme_lm_resource, list);
if (!bridge->lm_attach) {
- dev_err(bridge->parent, "vme_lm_attach not supported\n");
+ dev_err(bridge->parent, "%s not supported\n", __func__);
return -EINVAL;
}
@@ -1671,7 +1671,7 @@ int vme_lm_detach(struct vme_resource *resource, int monitor)
lm = list_entry(resource->entry, struct vme_lm_resource, list);
if (!bridge->lm_detach) {
- dev_err(bridge->parent, "vme_lm_detach not supported\n");
+ dev_err(bridge->parent, "%s not supported\n", __func__);
return -EINVAL;
}
@@ -1738,7 +1738,7 @@ int vme_slot_num(struct vme_dev *vdev)
}
if (!bridge->slot_get) {
- dev_warn(bridge->parent, "vme_slot_num not supported\n");
+ dev_warn(bridge->parent, "%s not supported\n", __func__);
return -EINVAL;
}
--
2.42.0
Hi Soumya,
On Tue, Oct 17, 2023 at 09:36:33PM -0700, Soumya Negi wrote:
> Replace function names in message strings with __func__ to fix
> all checkpatch warnings like:
>
> WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
> this function's name, in a string
>
> Signed-off-by: Soumya Negi <[email protected]>
> ---
> drivers/staging/vme_user/vme.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> index e8c2c1e77b7d..11c1df12b657 100644
> --- a/drivers/staging/vme_user/vme.c
> +++ b/drivers/staging/vme_user/vme.c
> @@ -422,7 +422,7 @@ int vme_slave_get(struct vme_resource *resource, int *enabled,
> image = list_entry(resource->entry, struct vme_slave_resource, list);
>
> if (!bridge->slave_get) {
> - dev_err(bridge->parent, "vme_slave_get not supported\n");
> + dev_err(bridge->parent, "%s not supported\n", __func__);
> return -EINVAL;
> }
>
> @@ -572,7 +572,7 @@ int vme_master_set(struct vme_resource *resource, int enabled,
> image = list_entry(resource->entry, struct vme_master_resource, list);
>
> if (!bridge->master_set) {
> - dev_warn(bridge->parent, "vme_master_set not supported\n");
> + dev_warn(bridge->parent, "%s not supported\n", __func__);
I wouldn't disagree if you made this dev_err() instead of
dev_warn(). The reasoning behind is that if it's a warning you
should not fail. But beacuse you are returning -EINVAL it means
that you are failing, therefore you should use dev_err().
Others might object that the change I'm suggesting sohuld go in a
different patch, which is also OK.
> return -EINVAL;
... or, if you want to keep the dev_warn(), whou can consider
removing the "return -EINVAL;". But this is an evaluation you
should make in a different patch and mainly evaluate if it's
OK to remove the error here.
> }
>
> @@ -1565,7 +1565,7 @@ int vme_lm_set(struct vme_resource *resource, unsigned long long lm_base,
> lm = list_entry(resource->entry, struct vme_lm_resource, list);
>
> if (!bridge->lm_set) {
> - dev_err(bridge->parent, "vme_lm_set not supported\n");
> + dev_err(bridge->parent, "%s not supported\n", __func__);
> return -EINVAL;
> }
>
> @@ -1601,7 +1601,7 @@ int vme_lm_get(struct vme_resource *resource, unsigned long long *lm_base,
> lm = list_entry(resource->entry, struct vme_lm_resource, list);
>
> if (!bridge->lm_get) {
> - dev_err(bridge->parent, "vme_lm_get not supported\n");
> + dev_err(bridge->parent, "%s not supported\n", __func__);
> return -EINVAL;
> }
>
> @@ -1638,7 +1638,7 @@ int vme_lm_attach(struct vme_resource *resource, int monitor,
> lm = list_entry(resource->entry, struct vme_lm_resource, list);
>
> if (!bridge->lm_attach) {
> - dev_err(bridge->parent, "vme_lm_attach not supported\n");
> + dev_err(bridge->parent, "%s not supported\n", __func__);
> return -EINVAL;
> }
>
> @@ -1671,7 +1671,7 @@ int vme_lm_detach(struct vme_resource *resource, int monitor)
> lm = list_entry(resource->entry, struct vme_lm_resource, list);
>
> if (!bridge->lm_detach) {
> - dev_err(bridge->parent, "vme_lm_detach not supported\n");
> + dev_err(bridge->parent, "%s not supported\n", __func__);
> return -EINVAL;
> }
>
> @@ -1738,7 +1738,7 @@ int vme_slot_num(struct vme_dev *vdev)
> }
>
> if (!bridge->slot_get) {
> - dev_warn(bridge->parent, "vme_slot_num not supported\n");
> + dev_warn(bridge->parent, "%s not supported\n", __func__);
> return -EINVAL;
> }
Nothing wrong with the patch itself. But imagine if we end up in
one of those printouts and, as a user, you read something like:
... vme_slot_num not supported
The message itself doesn't say much to the user. The perfect fix
would be to re-write all these error messages with a proper
meaningful sentence, like, e.g.:
Can't retrieve the CS/CSR slot id
(don't even know if it's fully correct). Anyway, I understand
you don't have much time for such fine changes, so whatever you
decide to do:
Acked-by: Andi Shyti <[email protected]>
Andi
Hi Andi,
On Wed, Oct 18, 2023 at 01:00:01PM +0200, Andi Shyti wrote:
> Hi Soumya,
>
> On Tue, Oct 17, 2023 at 09:36:33PM -0700, Soumya Negi wrote:
> > Replace function names in message strings with __func__ to fix
> > all checkpatch warnings like:
> >
> > WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
> > this function's name, in a string
> >
> > Signed-off-by: Soumya Negi <[email protected]>
> > ---
> > drivers/staging/vme_user/vme.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > index e8c2c1e77b7d..11c1df12b657 100644
> > --- a/drivers/staging/vme_user/vme.c
> > +++ b/drivers/staging/vme_user/vme.c
> > @@ -422,7 +422,7 @@ int vme_slave_get(struct vme_resource *resource, int *enabled,
> > image = list_entry(resource->entry, struct vme_slave_resource, list);
> >
> > if (!bridge->slave_get) {
> > - dev_err(bridge->parent, "vme_slave_get not supported\n");
> > + dev_err(bridge->parent, "%s not supported\n", __func__);
> > return -EINVAL;
> > }
> >
> > @@ -572,7 +572,7 @@ int vme_master_set(struct vme_resource *resource, int enabled,
> > image = list_entry(resource->entry, struct vme_master_resource, list);
> >
> > if (!bridge->master_set) {
> > - dev_warn(bridge->parent, "vme_master_set not supported\n");
> > + dev_warn(bridge->parent, "%s not supported\n", __func__);
>
> I wouldn't disagree if you made this dev_err() instead of
> dev_warn(). The reasoning behind is that if it's a warning you
> should not fail. But beacuse you are returning -EINVAL it means
> that you are failing, therefore you should use dev_err().
>
> Others might object that the change I'm suggesting sohuld go in a
> different patch, which is also OK.
>
> > return -EINVAL;
>
> ... or, if you want to keep the dev_warn(), whou can consider
> removing the "return -EINVAL;". But this is an evaluation you
> should make in a different patch and mainly evaluate if it's
> OK to remove the error here.
I think it should be dev_err() too. The driver has inconsistently used
warn and err log levels in similar functions as well. But I was planning
to tackle those in a standalone patch after the printk's are gone. Or do
you think it should be part of this patchset?
> > if (!bridge->slot_get) {
> > - dev_warn(bridge->parent, "vme_slot_num not supported\n");
> > + dev_warn(bridge->parent, "%s not supported\n", __func__);
> > return -EINVAL;
> > }
>
> Nothing wrong with the patch itself. But imagine if we end up in
> one of those printouts and, as a user, you read something like:
>
> ... vme_slot_num not supported
>
> The message itself doesn't say much to the user. The perfect fix
> would be to re-write all these error messages with a proper
> meaningful sentence, like, e.g.:
>
> Can't retrieve the CS/CSR slot id
>
> (don't even know if it's fully correct). Anyway, I understand
> you don't have much time for such fine changes, so whatever you
> decide to do:
Got it. Thanks for the patch suggestion. Although yes, since I still have project
starter tasks pending for my outreachy application(will have to prioritize
them) I'm not sure if I will be able to get this done right away. Will try to
though.
> Acked-by: Andi Shyti <[email protected]>
>
> Andi
Thanks for the ack. Since I'll have to revise and resend this patch as v2,
should I not add your ack tag as the patch will be reviewed again? Just
wondering what the etiquette is.
Though normally are patches supposed to be resent as new versions when adding
ack tags?
Regards,
Soumya
Hi Soumya,
> > On Tue, Oct 17, 2023 at 09:36:33PM -0700, Soumya Negi wrote:
> > > Replace function names in message strings with __func__ to fix
> > > all checkpatch warnings like:
> > >
> > > WARNING: Prefer using '"%s...", __func__' to using 'vme_lm_get',
> > > this function's name, in a string
> > >
> > > Signed-off-by: Soumya Negi <[email protected]>
> > > ---
> > > drivers/staging/vme_user/vme.c | 14 +++++++-------
> > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > > index e8c2c1e77b7d..11c1df12b657 100644
> > > --- a/drivers/staging/vme_user/vme.c
> > > +++ b/drivers/staging/vme_user/vme.c
> > > @@ -422,7 +422,7 @@ int vme_slave_get(struct vme_resource *resource, int *enabled,
> > > image = list_entry(resource->entry, struct vme_slave_resource, list);
> > >
> > > if (!bridge->slave_get) {
> > > - dev_err(bridge->parent, "vme_slave_get not supported\n");
> > > + dev_err(bridge->parent, "%s not supported\n", __func__);
> > > return -EINVAL;
> > > }
> > >
> > > @@ -572,7 +572,7 @@ int vme_master_set(struct vme_resource *resource, int enabled,
> > > image = list_entry(resource->entry, struct vme_master_resource, list);
> > >
> > > if (!bridge->master_set) {
> > > - dev_warn(bridge->parent, "vme_master_set not supported\n");
> > > + dev_warn(bridge->parent, "%s not supported\n", __func__);
> >
> > I wouldn't disagree if you made this dev_err() instead of
> > dev_warn(). The reasoning behind is that if it's a warning you
> > should not fail. But beacuse you are returning -EINVAL it means
> > that you are failing, therefore you should use dev_err().
> >
> > Others might object that the change I'm suggesting sohuld go in a
> > different patch, which is also OK.
> >
> > > return -EINVAL;
> >
> > ... or, if you want to keep the dev_warn(), whou can consider
> > removing the "return -EINVAL;". But this is an evaluation you
> > should make in a different patch and mainly evaluate if it's
> > OK to remove the error here.
>
> I think it should be dev_err() too. The driver has inconsistently used
> warn and err log levels in similar functions as well. But I was planning
> to tackle those in a standalone patch after the printk's are gone. Or do
> you think it should be part of this patchset?
OK... if you already noticed this and you are saying there are
more cases, then it's better if you send a different patch.
> > > if (!bridge->slot_get) {
> > > - dev_warn(bridge->parent, "vme_slot_num not supported\n");
> > > + dev_warn(bridge->parent, "%s not supported\n", __func__);
> > > return -EINVAL;
> > > }
> >
> > Nothing wrong with the patch itself. But imagine if we end up in
> > one of those printouts and, as a user, you read something like:
> >
> > ... vme_slot_num not supported
> >
> > The message itself doesn't say much to the user. The perfect fix
> > would be to re-write all these error messages with a proper
> > meaningful sentence, like, e.g.:
> >
> > Can't retrieve the CS/CSR slot id
> >
> > (don't even know if it's fully correct). Anyway, I understand
> > you don't have much time for such fine changes, so whatever you
> > decide to do:
>
> Got it. Thanks for the patch suggestion. Although yes, since I still have project
> starter tasks pending for my outreachy application(will have to prioritize
> them) I'm not sure if I will be able to get this done right away. Will try to
> though.
yeah... nevermind.
> > Acked-by: Andi Shyti <[email protected]>
> >
> > Andi
>
> Thanks for the ack. Since I'll have to revise and resend this patch as v2,
> should I not add your ack tag as the patch will be reviewed again? Just
> wondering what the etiquette is.
You can keep my tag, unless you are making drastic changes.
Nevertheless, the patch might be reviewed again.
> Though normally are patches supposed to be resent as new versions when adding
> ack tags?
If someones acks or reviews the patches you don't need to resend
just for adding the tags. There are tools doing it and
maintainers take care of that.
But, on the other hand, if you are going to resend anyway, for
other reasons a new version, then you should add the tags
yourself, otherwise they might be lost.
If the new version changes considerably, then you might consider
not adding any tags.
Andi