lkml.org 
[lkml]   [2012]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 15/17] powerpc: crypto: sysfs routines and docs for the nx device driver
    From
    Date
    Hi Greg,

    On Wed, 2012-03-21 at 15:11 -0700, Greg KH wrote:
    > On Wed, Mar 21, 2012 at 04:41:20PM -0500, Kent Yoder wrote:
    > > These routines add sysfs files supporting the Power7+ in-Nest encryption
    > > accelerator driver.
    > >
    > > Signed-off-by: Kent Yoder <key@linux.vnet.ibm.com>
    > > ---
    > > Documentation/powerpc/pfo-nx-crypto.txt | 52 ++++++++
    >
    > Please put sysfs file information in Documentation/ABI/ where it
    > belongs.

    Will do, I see debugfs docs in there too.

    > Shouldn't the first just be debugfs files, as no "normal" user will ever
    > care about such a thing?
    >
    > Actually, why are these sysfs files at all, how about all of this going
    > into debugfs?

    Yes, that's fine. These really are just for checking 'am I really
    doing hardware encryption' and debugging any return codes that may come
    back from the hcall. The error return is probably more easily dev_err'd
    though.

    >
    > > +Error Detection
    > > +===============
    >
    > <snip>
    >
    > What can anyone do with any of these files? What use are they to users?
    >
    > > +Device Use
    > > +==========
    >
    > Again, what does a user care about these items for?
    >
    > > +int
    > > +nx_sysfs_init(struct device_driver *drv)
    > > +{
    > > + int rc;
    > > +
    > > + rc = driver_create_file(drv, &driver_attr_aes_ops);
    > > + if (rc)
    > > + goto out;
    >
    > <snip>
    >
    > Oh, ${DIETY}, no. Please don't create files one by one, we do have
    > functions that do all of this for you automatically, why aren't you
    > using them?

    Ok, I'll go look for some debugfs wrappers.

    > > +void
    > > +nx_sysfs_fini(struct device_driver *drv)
    > > +{
    > > + driver_remove_file(drv, &driver_attr_sync_ops);
    > > + driver_remove_file(drv, &driver_attr_aes_bytes);
    > > + driver_remove_file(drv, &driver_attr_aes_ops);
    > > + driver_remove_file(drv, &driver_attr_sha256_bytes);
    > > + driver_remove_file(drv, &driver_attr_sha256_ops);
    > > + driver_remove_file(drv, &driver_attr_sha512_bytes);
    > > + driver_remove_file(drv, &driver_attr_sha512_ops);
    >
    > Same here, don't do this, do it all at once.
    >
    > > +}
    >
    > Who is calling these functions? Where in the device lifecycle are the
    > files being created? Did you just race userspace with how they are
    > created, or are you doing it "properly"? (hint, odds are, as you are
    > trying to manually create and remove these by hand, you aren't doing it
    > properly...)

    We're not racing here, other than with cat. Just so an admin can see
    his workload is getting offloaded.

    Thanks,
    Kent

    > thanks,
    >
    > greg k-h
    >




    \
     
     \ /
      Last update: 2012-03-21 23:47    [W:0.024 / U:29.868 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site