Sorry, sent it from not original mail.
On 16.04.15 20:27, subscivan wrote:
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.
--
Regards,
Ivan Khoronzhuk
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
