Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables

[subscivan]

Jean,

On 16.04.15 18:44, Jean Delvare wrote:
> Hi Ivan,
>
> Le Thursday 16 April 2015 Ã 15:56 +0300, Ivan.khoronzhuk a Ãcrit :
> > On 16.04.15 12:52, Jean Delvare wrote:
> > > On Thu,  2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:
> > > > +static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
> > > This one could be world-readable as it contains no sensitive
> > > information.
> > It contains the address of DMI table containing sensitive information.
> > Who knows which ways can be used to take it. Anyway, no see reasons in this
> > w/o DMI table. But if you insist I can do it "world-readable".
> OK, you convinced me.
>
> > > > +struct bin_attribute bin_attr_dmi_table =
> > > > +			__BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
> > > I do not understand why you don't use BIN_ATTR here too? I tried naming
> > > the attribute bin_attr_DMI and it seems to work just fine, checkpatch
> > > doesn't even complain!
> > I dislike upper case in names, at least in such simple names.
> > It makes me using bin_attr_DMI lower in the code. That's why.
> > But if you like it, I will name it "bin_attr_DMI"
> I don't like upper case in names either, but in this specific case, I'd
> do it for consistency. As you wish though, I really only wanted to know
> the reason.
>
> > > > +
> > > > +static int __init dmi_init(void)
> > > > +{
> > > > +	int ret = -ENOMEM;
> > > > +	struct kobject *tables_kobj = NULL;
> > > > +
> > > > +	if (!dmi_available) {
> > > > +		ret = -ENOSYS;
> > > > +		goto err;
> > > > +	}
> > > This is weird. Can this actually happen?
> > >
> > > We currently have two ways to enter this module: dmi_scan_machine(),
> > > which is called by the architecture code, and dmi_init(), which is
> > > called at subsys_initcall time. As far as I can see,
> > > core/arch_initcalls are guaranteed to be always called before
> > > subsys_initcalls. If we can rely on that, the test above is not needed.
> > > If for any reason we can't, that means that dmi_init() should not be a
> > > subsys_initcall, but should instead be called explicitly at the end of
> > > dmi_scan_machine().
> > We cannot be sure that firmware_kobj created at time of dmi_init().
> > The sources don't oblige you to call it at core level,
> > for instance like it was done for arm64. For x86, dmi_init() can be called
> > before firmware_kobj is created.
> Looking at the code, it seems that firmware_kobj is created very, very
> early in the boot process. In do_basic_setup(), you can see that
> driver_init() (which in turn calls firmware_init(), creating
> firmware_kobj) is called before do_initcalls(). So firmware_kobj must be
> defined before dmi_scan_machine() or dmi_init() is called.

No. Not must, rather should. See below.

> Oh, and this wasn't even my point ;-) I'm fine with you checking if
> firmware_kobj is defined. My question was about the dmi_available check
> above. But that question was silly anyway, sorry. I confused
> dmi_available with dmi_initialized. Checking for dmi_available is
> perfectly reasonable, please scratch my objection.
>
> > And if I call it from dmi_init() I suppose
> > I would face an error. As I can't call it in dmi_init I can't be sure that
> > DMI is available at all. So, no, we have to check dmi_available here and
> > call it at subsys layer, where it's supposed to be.
> I can't parse that, I suspect you wrote dmi_init where you actually
> meant dmi_scan_machine? Given how early firmware_kobj is created, I
> think the code currently in dmi_init could in fact go at the end of
> dmi_scan_machine.

Actually, dmi_scan_machine can be called even earlier.
As I've sad, for x86, it's called before firmware_kobj is created.

kernel_start()
    setup_arch()
        dmi_scan_machine()

And for firmware_init(), as you noticed already:

start_kernel()
    rest_init()
        kernel_init()
            kernel_init_freeable()
                do_basic_setup()
                    driver_init()
                        firmware_init()

Pay attentions that setup_arch() is called much earlier than rest_init().
So dmi_init couldn't in fact go at the end of dmi_scan_machine.

> But it's not important for the time being, this can be
> attempted later.
>
> > > > (...)
> > > > +	kobject_del(dmi_kobj);
> > > > +	kobject_put(dmi_kobj);
> > > > +	dmi_kobj = NULL;
> > > I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
> > > appropriate comment), so that dmi-sysfs has a chance to load? As it is
> > > now, a bug or some unexpected behavior in this new code could cause a
> > > regression for dmi-sysfs users. Just because I don't like dmi_sysfs
> > > doesn't mean we can break it ;-)
> > As I remember it was not so critical for you last time.
> > "I don't care which way you choose". And I've explained my position.
> > But it's not very hard to me to change it. Anyway patch requires re-push.
> You're right, I did not remember we had discussed this already,
> sorry :-(
>
> Well, I still agree that it doesn't really matter, but of two acceptable
> solutions for an event which will most likely never happen, why not go
> for the ones with the fewer lines of code? ;-)

Ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/