Re: [PATCH] staging:vme_user:fix the issue of using the wrong error code

[piro yang]

piro yang <piroyangg@xxxxxxxxx> 于2023年12月12日周二 00:13写道：
>
> Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 于2023年12月12日周二 00:01写道：
> >
> > On Mon, Dec 11, 2023 at 11:54:53PM +0800, Piro Yang wrote:
> > > 1. the error code of ENOSYS indicates Invalid system call number,
> > >    but there is not system call
> > >
> > > 2. unified the logging error message, when slave_set func is NULL
> >
> > That is two different things, and as such, should be two different
> > changes, right?
> >
> > Yes, it's picky, but that's what the staging driver code is for, to get
> > comfortable doing kernel development changes properly.
> >
> > Also, are you sure this second change is correct:
>
>    yes, I'm sure .
>    the vme_slave_set function is diffierent
> /*****************************vme_slave_set***********************/
> int vme_slave_set(struct vme_resource *resource, int enabled,
>                   unsigned long long vme_base, unsigned long long size,
>                   dma_addr_t buf_base, u32 aspace, u32 cycle)
> {
>         ...
>         if (!bridge->slave_set) {
>                  dev_err(bridge->parent, "Function not supported\n");
>                 return -ENOSYS;
>         }
>
> /******************other functions like below: *******************/
> int vme_slave_get(struct vme_resource *resource, int *enabled,
>                   unsigned long long *vme_base, unsigned long long *size,
>                   dma_addr_t *buf_base, u32 *aspace, u32 *cycle)
> {
>         ....
>         if (!bridge->slave_get) {
>                 dev_err(bridge->parent, "%s not supported\n", __func__);
>                 return -EINVAL;
>         }
>
> int vme_master_set(struct vme_resource *resource, int enabled,
>                    unsigned long long vme_base, unsigned long long size,
>                    u32 aspace, u32 cycle, u32 dwidth)
> {
>        ....
>         if (!bridge->master_set) {
>                 dev_warn(bridge->parent, "%s not supported\n", __func__);
>                 return -EINVAL;
>         }
>
> int vme_master_get(struct vme_resource *resource, int *enabled,
>                    unsigned long long *vme_base, unsigned long long *size,
>                    u32 *aspace, u32 *cycle, u32 *dwidth)
> {
>       ....
>         if (!bridge->master_get) {
>                 dev_warn(bridge->parent, "%s not supported\n", __func__);
>                 return -EINVAL;
>         }
>
> >
> > >
> > > Signed-off-by: Piro Yang <piroyangg@xxxxxxxxx>
> > > ---
> > >  drivers/staging/vme_user/vme.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > > index 5c416c31ec57..e9461a7a7ab8 100644
> > > --- a/drivers/staging/vme_user/vme.c
> > > +++ b/drivers/staging/vme_user/vme.c
> > > @@ -340,8 +340,8 @@ int vme_slave_set(struct vme_resource *resource, int enabled,
> > >       image = list_entry(resource->entry, struct vme_slave_resource, list);
> > >
> > >       if (!bridge->slave_set) {
> > > -             dev_err(bridge->parent, "Function not supported\n");
> > > -             return -ENOSYS;
> > > +             dev_err(bridge->parent, "%s not supported\n", __func__);
> >
> > __func__ is not the same here as "Function", right?  "Function" is the
> > functionality of the thing that is attempted here, so replacing that
> > word with the function name seems odd to me, don't you think?
> >
    yes, __func__ is not the same here as "Function".
    but I think the logging error message format  shoud be unified
with other functions.
    like:  vme_slave_get(){
             .....
             if (!bridge->slave_get) {
                dev_err(bridge->parent, "%s not supported\n", __func__);


> > thanks,
> >
> > greg k-h