lkml.org 
[lkml]   [2023]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v2] nubus: Don't list card resources by default
    On Thu, 23 Mar 2023, Geert Uytterhoeven wrote:

    > On Thu, Mar 23, 2023 at 7:02 AM Finn Thain <fthain@linux-m68k.org> wrote:
    > > Some Nubus cards have many ROM resources. A single Radius video card
    > > produced well over a thousand entries under /proc/bus/nubus/. Populating
    > > /proc/bus/nubus on a slow machine with several such cards installed takes
    > > long enough that the user may think that the system is wedged. All those
    > > procfs entries also consume significant RAM though they are not normally
    > > needed (except by developers). Omit these resources from /proc/bus/nubus/
    > > by default and add a kernel parameter to enable them when needed.
    > > On the test machine, this saved 300 kB and 10 seconds.
    > >
    > > Tested-by: Stan Johnson <userm57@yahoo.com>
    > > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
    >
    > Thanks for your patch!
    >

    Thanks for your review.

    > > Checkpatch says "externs should be avoided in .c files" and if this one
    > > appeared twice I would agree. But as it only appears once, I can't see
    > > any advantage to putting it in a new .h file instead of the .c file...
    >
    > The advantage is that it allows the compiler to check that the
    > signatures of the declaration and the definition do match, now and in
    > the future.
    >

    Ah yes, I forgot that we use a header file to make sure that the header
    file is correct ;-)

    > > --- a/drivers/nubus/nubus.c
    > > +++ b/drivers/nubus/nubus.c
    > > @@ -34,6 +34,9 @@
    > >
    > > LIST_HEAD(nubus_func_rsrcs);
    > >
    > > +bool procfs_rsrcs;
    > > +module_param(procfs_rsrcs, bool, 0444);
    >
    > With the expanded functionality, is "rsrcs" still a good name?
    > Perhaps this should be an integer, so you can define different
    > levels? E.g.
    > - 0 = just devices
    > - 1 = above + boards + public resources
    > - 2 = above + private resources

    That really depends on how the proc entries get used. I think it's
    probably going to be developers who would use them so I consider all of
    this to be outside of the UAPI and subject to change. But it would be nice
    to hear from other developers on that point.

    Regarding terminology, the files in /proc/bus/nubus/*/ are termed
    "records" or "entries" while the subdirectories may represent boards, slot
    resources or tables of entries. So a parameter like "proc_entries" (in
    effect nubus.proc_entries) might be more apt than "procfs_rsrcs".

    Linux "devices" correspond to the "functional resources" offered by a
    card. (Other resources have other purposes.)

    I don't know where the "local/private" designation originates from. It's
    not to be found in Apple's book, "Designing Cards and Drivers for the
    Macintosh". AFAIK, there's no distinction between "public" and "private"
    like you might expect to find between the slot resources needed by Apple's
    Slot Manager and those needed by 3rd party drivers. E.g. the
    NUBUS_RESID_MAC_ADDRESS and NUBUS_RESID_GAMMADIR slot resources were
    Apple-defined, even though nubus.c describes them as "local/private".

    Note that this patch doesn't affect the /proc/bus/nubus/boards file which
    lists category, type, dr_sw, dr_hw for every functional resource
    implemented by every board.

    > (disclaimer: I know nothing about NuBus and the current /proc/nubus
    > layout)

    Here's an example from my LCIII. There's only one card (in slot e) and it
    has minimal resources and entries. e/1 is the "vendor info" slot resource,
    e/1/3 is the "revision" entry from that slot resource.

    # grep -ar . /proc/bus/nubus
    /proc/bus/nubus/e/1/1:
    /proc/bus/nubus/e/1/2:Ethernet A-Series
    /proc/bus/nubus/e/1/20:
    /proc/bus/nubus/e/1/24/1:A-Series
    /proc/bus/nubus/e/1/24/3:001
    /proc/bus/nubus/e/1/24/4:SS DEV
    /proc/bus/nubus/e/80/1:

    /proc/bus/nubus/e/80/2:Network_Ethernet_A_Series
    /proc/bus/nubus/e/80/a:
    /proc/bus/nubus/e/80/80:[snipped binary MAC address]
    /proc/bus/nubus/devices:e 0004 0001 010c 0100 fe000000
    #

    With this patch, only the /proc/bus/nubus/devices file appears by default.
    nubus.c captures that information for bus matching purposes, but the
    /proc/bus/nubus/e/* inodes are unused AFAIK. I think they were put there
    so that userland programs could find uses for them. Problem is, it can be
    expensive to do that, depending on the card ROM, and hence this patch.

    >
    > Should this be documented? I know there is currently nothing about NuBus
    > under Documentation/.
    >

    Probably...

    > > +
    > > /* Meaning of "bytelanes":
    > >
    > > The card ROM may appear on any or all bytes of each long word in
    > > @@ -574,7 +577,9 @@ nubus_get_functional_resource(struct nubus_board *board, int slot,
    > > default:
    > > /* Local/Private resources have their own
    > > function */
    > > - nubus_get_private_resource(fres, dir.procdir, &ent);
    > > + if (procfs_rsrcs)
    > > + nubus_get_private_resource(fres, dir.procdir,
    > > + &ent);
    > > }
    > > }
    > >
    > > diff --git a/drivers/nubus/proc.c b/drivers/nubus/proc.c
    > > index 2c320a84fd72..844e86636798 100644
    > > --- a/drivers/nubus/proc.c
    > > +++ b/drivers/nubus/proc.c
    > > @@ -51,11 +51,13 @@ static struct proc_dir_entry *proc_bus_nubus_dir;
    > > * /proc/bus/nubus/x/ stuff
    > > */
    > >
    > > +extern bool procfs_rsrcs;
    > > +
    > > struct proc_dir_entry *nubus_proc_add_board(struct nubus_board *board)
    > > {
    > > char name[2];
    > >
    > > - if (!proc_bus_nubus_dir)
    > > + if (!proc_bus_nubus_dir || !procfs_rsrcs)
    > > return NULL;
    > > snprintf(name, sizeof(name), "%x", board->slot);
    > > return proc_mkdir(name, proc_bus_nubus_dir);
    > > @@ -72,7 +74,7 @@ struct proc_dir_entry *nubus_proc_add_rsrc_dir(struct proc_dir_entry *procdir,
    > > char name[9];
    > > int lanes = board->lanes;
    > >
    > > - if (!procdir)
    > > + if (!procdir || !procfs_rsrcs)
    > > return NULL;
    > > snprintf(name, sizeof(name), "%x", ent->type);
    > > remove_proc_subtree(name, procdir);
    > > @@ -157,7 +159,7 @@ void nubus_proc_add_rsrc_mem(struct proc_dir_entry *procdir,
    > > char name[9];
    > > struct nubus_proc_pde_data *pded;
    > >
    > > - if (!procdir)
    > > + if (!procdir || !procfs_rsrcs)
    > > return;
    > >
    > > snprintf(name, sizeof(name), "%x", ent->type);
    > > @@ -176,7 +178,7 @@ void nubus_proc_add_rsrc(struct proc_dir_entry *procdir,
    > > char name[9];
    > > unsigned char *data = (unsigned char *)ent->data;
    > >
    > > - if (!procdir)
    > > + if (!procdir || !procfs_rsrcs)
    > > return;
    > >
    > > snprintf(name, sizeof(name), "%x", ent->type);
    >
    \
     
     \ /
      Last update: 2023-03-27 01:15    [W:3.685 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site