Re: [PATCH v2] nubus: Don't list card resources by default

[Finn Thain <fthain@linux-m68k.org>]

[-- Attachment #1: Type: text/plain, Size: 6892 bytes --]

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);
>