On 31/12/14 15:05, Andy Shevchenko wrote:
<snip>
>> +int imr_del(int reg, unsigned long base, unsigned long size);
>
> Same comments as for add_range.
> Could it be imr_remove_range() ?
Can be. You've actually caught me out there, function name is "imr_del"
function description says imr_del_range(). Amazing to think I proof read
this file a number of times for exactly thing type of thing
I'm happy with
imr_add_range();
imr_remove_range();
>> +
>> +#endif /* _IMR_H */
>> diff --git a/arch/x86/include/asm/intel-quark.h
b/arch/x86/include/asm/intel-quark.h
>> new file mode 100644
>> index 0000000..f51ac8c
>> --- /dev/null
>> +++ b/arch/x86/include/asm/intel-quark.h
<snip>
>> +#ifdef CONFIG_X86_32
>> +static inline int cpu_is_quark(const struct cpuinfo_x86 *c)
>> +{
>> + return (c->x86_vendor == X86_VENDOR_INTEL &&
>> + c->x86 == 5 && c->x86_model == 9);
>> +}
>> +#else
>> +static inline int cpu_is_quark(const struct cpuinfo_x86 *c)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +#endif /* _ASM_X86_INTEL_QUARK_H */
>
> Could we use x86_match_cpu() instead?
I don't see why not. I'll kill this header and call out to
x86_match_cpu() in the Galileo and IMR code.
>> + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
>> + reg, &imr->addr_lo);
>> + if (ret)
>> + return ret;
>> +
>> + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
>> + ++reg, &imr->addr_hi);
>
> Could you use reg++ in previous line and so on?
Yes I could. Originally wrote the code with a pre-increment and then
changed the flow later on. Post-increment will work just as good now and
is more readable.
> One more idea, if you make a union inside the structure you may do
> this in a loop.
> struct imr {
> union {
> struct imr {
> ...
> };
> u32 regs[IMR_NUM_REGS];
> };
> }
> Mostly same for _write().
Since reg is incrementing on each iosf_mbi_dothing() that can work.
>> + u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS);
>> + u32 reg_lock = reg;
>
> Do we need a separate variable? Would (reg - IMR_NUM_REGS) work for you?
Sure. A separate integer isn't required, I can make that change.
>> +done:
>> + local_irq_restore(flags);
>
> Could you do like
>
> local_irq_restore(flags);
> return 0;
> done:
> local_irq_restore(flags);
> WARN(...)
> return ret;
> ?
Doesn't change the logic so yeah, works for me.
>> +static int imr_dbgfs_state_show(struct seq_file *s, void *unused)
>> +{
>> + int i, ret = -ENODEV;
>> + struct imr imr;
>> + u32 size;
>> +
>> + mutex_lock(&imr_dev.lock);
>
> It seems you may get the imr_dev via parameters. I suggest to avoid
> using global variables as much as possible.
Appreciated, you're right globals are bad.
I think in this case the mutex at that scope is necessary though. Same
with the debugfs handle.
>> +
>> + for (i = 0; i <= imr_dev.num; i++) {
>
> num is not num, but last one? Sounds confusing for me.
Will change.
>> +
>> + f = debugfs_create_file("state", S_IFREG | S_IRUGO,
>> + dir, imr_dev, &imr_state_ops);
>> + if (!f)
>> + goto err;
>
> Are you planing to extend the debugfs contents? Would it be okay to
> use only one file for now?
No not planning to extend.
OK with the one file.
>> +}
>
> No need to put this function under ifdef - debugfs has the stubs.
Ah - wasn't aware of that.
Great stuff - I'll kill the ifdefs - thanks !
> Can you define pr_fmt() ?
>
> 1 KiB.
Yep will do
:g/pr_info/s//pr_fmt/g
>
>> + base, size);
>> + dump_stack();
>
> dump_stack is really needed here? In that case why not to use WARN()?
Hmm - I nope - dump_stack() isn't relevant since it's an in-kernel API.
Did an unthinking copy/past from the mtrr code
I'll zap that.
>> + if (imr_check_range(base, size))
>> + return -EINVAL;
>
> ret = imr_();
> if (ret)
> return ret;
Good catch.
>> + for (i = 0; i <= imr_dev.num; i++) {
>> + ret = imr_read(i, &imr);
>> + if (ret)
>> + goto done;
>> +
>> + /* Find overlap */
>> + if (imr_enabled(&imr)) {
>> + if (base >= imr_to_phys(imr.addr_lo) &&
>> + base <= imr_to_phys(imr.addr_hi)) {
>
>> + overlap = 1;
>> + break;
>
> Maybe
> ret = -EINVAL;
> goto done;
Agree - I like that change.
Waste of time to complete the loop if an overlap is detected.
>> + if (reg >= 0) {
>> + ret = imr_read(reg, &imr);
>> + if (ret)
>> + goto done;
>> +
>> + if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) {
>> + ret = -ENODEV;
>> + goto done;
>> + }
>> + found = 1;
>
> You may put a loop to the else branch instead of checking found at
> each iteration.
Sure.
> Happy New Year!
Thanks for the feedback Andy !
Best wishes in the New Year.
BOD
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
