Messages in this thread | | | Date | Thu, 16 Apr 2015 20:27:00 +0300 | From | subscivan <> | Subject | Re: [Patch 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables |
| |
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.
| |