Message-ID: <YwdBMIgSzEiFjc4D@kroah.com>
Date:   Thu, 25 Aug 2022 11:30:24 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     "Czerwacki, Eial" <eial.czerwacki@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Arsh, Leonid" <leonid.arsh@....com>,
        "Twaig, Oren" <oren.twaig@....com>,
        SAP vSMP Linux Maintainer <linux.vsmp@....com>,
        Arnd Bergmann <arnd@...db.de>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Andra Paraschiv <andraprs@...zon.com>,
        Borislav Petkov <bp@...e.de>,
        Brijesh Singh <brijesh.singh@....com>,
        Eric Biggers <ebiggers@...gle.com>, Fei Li <fei1.li@...el.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Jens Axboe <axboe@...nel.dk>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Subject: Re: [PATCH v2] drivers/virt/vSMP: new driver

On Thu, Aug 25, 2022 at 09:17:57AM +0000, Czerwacki, Eial wrote:
> Greetings Greg,
> 
> thank you for the comments, see my comment below.
> >On Thu, Aug 25, 2022 at 06:24:02AM +0000, Czerwacki, Eial wrote:
> >> Introducing the vSMP guest driver which allows interaction with the
> >> vSMP control device when running a Linux OS atop of the vSMP hypervisor.
> >> vSMP is a resource aggregation hypervisor from SAP.
> >> 
> >> The driver comprises of api part which facilitates communication with
> >> the hypervisor and version which displays the hypervisor's version.
> >> 
> >> This patch s based on previous patches sent to the staging tree mailing
> >> lists
> >> 
> >> Signed-off-by: Eial Czerwacki <eial.czerwacki@....com>
> >> Acked-by: Leonid Arsh <leonid.arsh@....com>
> >> Acked-by: Oren Twaig <oren.twaig@....com>
> >> CC: SAP vSMP Linux Maintainer <linux.vsmp@....com>
> >> CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> >> CC: Arnd Bergmann <arnd@...db.de>
> >> CC: Dan Carpenter <dan.carpenter@...cle.com>
> >> CC: Andra Paraschiv <andraprs@...zon.com>
> >> CC: Borislav Petkov <bp@...e.de>
> >> CC: Brijesh Singh <brijesh.singh@....com>
> >> CC: Eric Biggers <ebiggers@...gle.com>
> >> CC: Fei Li <fei1.li@...el.com>
> >> CC: Hans de Goede <hdegoede@...hat.com>
> >> CC: Jens Axboe <axboe@...nel.dk>
> >> CC: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> >> 
> >> v1 -> v2:
> >>        - fix -0 var init in add_sysfs_entries (pointed out by Dan Carpenter)
> >> ---
> >>  Documentation/ABI/stable/sysfs-driver-vsmp |   5 +
> >>  MAINTAINERS                                |   6 +
> >>  drivers/virt/Kconfig                       |   2 +
> >>  drivers/virt/Makefile                      |   2 +
> >>  drivers/virt/vsmp/Kconfig                  |  11 +
> >>  drivers/virt/vsmp/Makefile                 |   7 +
> >>  drivers/virt/vsmp/api/api.c                | 249 +++++++++++++++++++++
> >>  drivers/virt/vsmp/api/api.h                |  69 ++++++
> >>  drivers/virt/vsmp/include/registers.h      |  12 +
> >>  drivers/virt/vsmp/version/version.c        | 118 ++++++++++
> >>  drivers/virt/vsmp/version/version.h        |  14 ++
> >>  drivers/virt/vsmp/vsmp_main.c              | 110 +++++++++
> >>  12 files changed, 605 insertions(+)
> >
> >Why do you have all of these different .c and .h files for only 600
> >lines of code?  Shouldn't this all just be in a single .c file?  Why
> >have a subdir for just 300 lines?
> >
> >Please mush this all into a single .c file going forward, speading it
> >out like this makes no sense.
> the current driver has two modules, version and api, in later versions of the
> driver support for additional features will be added.
> at that time, this might cause the single file to inflate, so we thought to split it
> to several files to make it more organized.
> is there any way to keep it in different code files?

Keep it small and together now.  If you need to change it in the future,
wonderful, do it then.  Don't do things that we never know will happen
or not.

One file, and one module, is great to start with.  Please do that for
now.


> 
> >
> >>  create mode 100644 Documentation/ABI/stable/sysfs-driver-vsmp
> >>  create mode 100644 drivers/virt/vsmp/Kconfig
> >>  create mode 100644 drivers/virt/vsmp/Makefile
> >>  create mode 100644 drivers/virt/vsmp/api/api.c
> >>  create mode 100644 drivers/virt/vsmp/api/api.h
> >>  create mode 100644 drivers/virt/vsmp/include/registers.h
> >>  create mode 100644 drivers/virt/vsmp/version/version.c
> >>  create mode 100644 drivers/virt/vsmp/version/version.h
> >>  create mode 100644 drivers/virt/vsmp/vsmp_main.c
> >> 
> >> diff --git a/Documentation/ABI/stable/sysfs-driver-vsmp b/Documentation/ABI/stable/sysfs-driver-vsmp
> >> new file mode 100644
> >> index 000000000000..18a0a62f40ed
> >> --- /dev/null
> >> +++ b/Documentation/ABI/stable/sysfs-driver-vsmp
> >> @@ -0,0 +1,5 @@
> >> +What:           /sys/hypervisor/vsmp/version
> >> +Date:           Aug 2022
> >
> >August is almost over :(
> will fix, thank!
> 
> >
> >> +Contact:        Eial Czerwacki <eial.czerwacki@....com>
> >> +             linux-vsmp@....com
> >
> >No need for an alias here.
> it's not an alias, it is a shard mailbox for the team so others can
> view the mails history.
> I saw that the same methodology is done with mailing lists.

That's a mailing list?  Ah, didn't realize that, sorry, was not obvious.

> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index f512b430c7cb..cf74089c4d19 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -21783,6 +21783,12 @@ F:   lib/test_printf.c
> >>  F:   lib/test_scanf.c
> >>  F:   lib/vsprintf.c
> >> 
> >> +VSMP GUEST DRIVER
> >> +M:   Eial Czerwacki <eial.czerwacki@....com>
> >> +M:   linux-vsmp@....com
> >
> >Again, no random aliases please, stick to a person as a contact.
> see above comment

If this is a mailing list, mark it as such, you did not do so.


> >> +/*
> >> + * Register the vSMP sysfs object for user space interaction
> >> + */
> >> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
> >> +{
> >> +     int error = -EINVAL;
> >> +
> >> +     if (vsmp_sysfs_kobj && bin_attr) {
> >> +             error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);
> >
> >You raced userspace and lost :(
> not sure I understand, can you elaborate more please?

Fix up your sysfs usage first, and then we can revisit this.


> >
> >And why is your version file a binary file?  It should just be a small
> >text string, right?
> not so small, it can reach up to 512kb.

That was not obvious at all.  Please document this.

And how in the world is a "version" that big?  What exactly does this
contain?

> that is why I decided to go with binary, I understood that the text is rather limited.

That is true, sysfs is "one value per file", this can not be a file that
you parse.


> >> +int open_cfg_addr(struct pci_dev *pdev)
> >> +{
> >> +     u64 cfg_start;
> >> +     u32 cfg_len;
> >> +
> >> +     vsmp_dev_obj = pdev;
> >> +     cfg_start = pci_resource_start(vsmp_dev_obj, 0);
> >> +     cfg_len = pci_resource_len(vsmp_dev_obj, 0);
> >> +
> >> +     dev_dbg(get_dev(), "Mapping bar 0: [0x%llx,0x%llx]\n",
> >> +             cfg_start, cfg_start + cfg_len);
> >
> >Again, you have a device, use that.  Goes for the whole driver.
> I think I've missed something, please correct me if I'm wrong,
> I don't need to save the pdev because there is an existing framework
> that provides it?

You have pdev, use that in your dev_dbg() call.

> >> +#define FILE_PREM 0444
> >
> >File permission of what?  And shouldn't it be "PERM", not "PREM"?  And
> >why do you need it at all?  Just use the proper sysfs macros and you
> >never need it.  See below.
> all the sysfs files (1 for now) should be read for all users.

Then use the proper sysfs macros for that, do not set it yourself.

> I'll submit a new version after a proper file structure will be decided
> thanks again for you comments.

Again, stick with one file to start with.  You can always change that
later once it is merged if you really need it.

thanks,

greg k-h

Powered by blists - more mailing lists