Subject: Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
From: Jean Delvare <jdelvare@suse.de>
To: "Ivan.khoronzhuk" <ivan.khoronzhuk@globallogic.com>
Cc: linux-kernel@vger.kernel.org, matt.fleming@intel.com,
        ard.biesheuvel@linaro.org, grant.likely@linaro.org,
        linux-api@vger.kernel.org, linux-doc@vger.kernel.org, mikew@google.com,
        dmidecode-devel@nongnu.org, leif.lindholm@linaro.org,
        msalter@redhat.com, roy.franz@linaro.org
In-Reply-To: <552FB18D.6080207@globallogic.com>
References: <1427979423-22767-1-git-send-email-ivan.khoronzhuk@globallogic.com>
	 <1427979423-22767-3-git-send-email-ivan.khoronzhuk@globallogic.com>
	 <20150416115252.7dc964a3@endymion.delvare>
	 <552FB18D.6080207@globallogic.com>
Content-Type: text/plain; charset="UTF-8"
Organization: Suse Linux
Date: Thu, 16 Apr 2015 17:44:24 +0200
Message-ID: <1429199064.4386.93.camel@chaos.site>
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4860
Lines: 118

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.

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

> (...)
> Thanks Jean.
> I will add you propositions and re-push the series.
> If you are OK with my answer of-course.

You're welcome. Yes I think we have clarified everything now so you can
submit the next (hopefully last) iteration.

Thank you,
-- 
Jean Delvare
SUSE L3 Support

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