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:OK, you convinced me.
On Thu, 2 Apr 2015 15:57:02 +0300, Ivan Khoronzhuk wrote:It contains the address of DMI table containing sensitive information.
+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.
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".
I don't like upper case in names either, but in this specific case, I'dI dislike upper case in names, at least in such simple names.+struct bin_attribute bin_attr_dmi_table =I do not understand why you don't use BIN_ATTR here too? I tried naming
+ __BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
the attribute bin_attr_DMI and it seems to work just fine, checkpatch
doesn't even complain!
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"
do it for consistency. As you wish though, I really only wanted to know
the reason.
Looking at the code, it seems that firmware_kobj is created very, veryWe cannot be sure that firmware_kobj created at time of dmi_init().+This is weird. Can this actually happen?
+static int __init dmi_init(void)
+{
+ int ret = -ENOMEM;
+ struct kobject *tables_kobj = NULL;
+
+ if (!dmi_available) {
+ ret = -ENOSYS;
+ goto err;
+ }
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().
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.
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 supposeI can't parse that, I suspect you wrote dmi_init where you actually
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.
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.
You're right, I did not remember we had discussed this already,As I remember it was not so critical for you last time.(...)I'm wondering, wouldn't it make sense to keep dmi_kobj alive (with an
+ kobject_del(dmi_kobj);
+ kobject_put(dmi_kobj);
+ dmi_kobj = NULL;
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 ;-)
"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.
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.