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

[piro yang]

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?
>
> thanks,
>
> greg k-h