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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALaNatNpBJ6a-hv3MiV7NzuJ7wYLwHRCs3ppKM5O-UCvTSeiGg@mail.gmail.com>
Date:   Tue, 12 Dec 2023 00:13:05 +0800
From:   piro yang <piroyangg@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     linux-staging@...ts.linux.dev,
        Linux Outreachy <outreachy@...ts.linux.dev>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging:vme_user:fix the issue of using the wrong error code

Greg KH <gregkh@...uxfoundation.org> 于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@...il.com>
> > ---
> >  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