Re: [PATCH v2] drivers/virt/vSMP: new driver

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Aug 25, 2022 at 10:16:59AM +0000, Czerwacki, Eial wrote:
> >> >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.
> where should the document be?
> in the code as a comment or in another file?

In the Documentation/ABI/ file that describes this file.

> >And how in the world is a "version" that big?  What exactly does this
> >contain?
> it 's size depends on the number of resources it uses.
> here is an example:
> :~> cat /sys/hypervisor/vsmp/version  
> SAP vSMP Foundation: 10.6.2862.0 (Aug 22 2022 15:21:02)
> System configuration:
>    Boards:      2
>       1 x Proc. + I/O + Memory
>       1 x NVM devices (Amazon.com Amazon EC2 NVMe Instance Storage)
>    Processors:  1, Cores: 2, Threads: 4
>        Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz Stepping 04
>    Memory (MB): 30976 (of 103192), Cache: 7527, Private: 64689
>       1 x  6400MB    [ 7825/ 321/ 1104]      
>       1 x 24576MB    [95367/7206/63585]       00:1f.0#1
>    Boot device: [HDD] NVMe: Amazon Elastic Block Store        
> Supported until: Aug 22 2024

That is crazy, and is not a version.  It's a "configuration".

You have a version up there, just export that as a simple string
"10.6.2862.0".  What is the rest of that stuff needed for?  Who will use
it?  Should it just be in a debugfs file for debugging things?

Oh, and I love the "Supported until" tag, that's funny :)

> >> 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.
> should I keep it as bin then?

See above, make it text only for the version.  If you want to export
other things, be explicit and make them "one value per sysfs file" or
use debugfs for debugging things that no one relies on.

> >You have pdev, use that in your dev_dbg() call.
> I have pdev when the probe cb is called, however in other funcs I
> don't have it.

That's for the other functions to fix up.  Pass in the device pointer to
them.  You are a driver, there is no reason to NOT have a device pointer
in your functions, otherwise you are operating on global state which a
driver should never do.  You will notice that quickly when you remove
the static variable at the top of the file like I asked you to :)

thanks,

greg k-h