lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 2 Sep 2016 16:59:09 +0200 (CEST) From: Thomas Gleixner <tglx@...utronix.de> To: Craig Gallek <kraigatgoog@...il.com> cc: David Decotigny <decot@...gle.com>, LKML <linux-kernel@...r.kernel.org> Subject: Re: [PATCH RESEND] genirq: Machine-parsable version of /proc/interrupts On Tue, 26 Jul 2016, Craig Gallek wrote: > /* > * Core internal functions to deal with irq descriptors > @@ -92,6 +93,7 @@ struct irq_desc { > int parent_irq; > struct module *owner; > const char *name; > + struct kobject kobj; Lacks a docbook comment. > @@ -121,6 +122,145 @@ EXPORT_SYMBOL_GPL(nr_irqs); > static DEFINE_MUTEX(sparse_irq_lock); > static DECLARE_BITMAP(allocated_irqs, IRQ_BITMAP_BITS); > > +struct kobject *irq_kobj; That wants to be static. Aside of that the variable name sucks. irq_kobj_base or such would be self explaining. > +#define IRQ_ATTR_RO(_name) \ > +static struct kobj_attribute _name##_attr = __ATTR_RO(_name) All these show() functions want to be conditional on CONFIG_SYSFS > +static ssize_t per_cpu_count_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj); > + int i; > + unsigned long flags; > + ssize_t ret = 0; Can you please order the variables as we do everywhere else? + struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj); + unsigned long flags; + ssize_t ret = 0; + int i; Hmm? > + > + raw_spin_lock_irqsave(&desc->lock, flags); Why would we need locking for this? The count is unsigned int, so you can access it lockless. > + for_each_online_cpu(i) { > + unsigned int c = kstat_irqs_cpu(desc->irq_data.irq, i); Missing new line between variable declaration and code. > + if (!ret) > + ret = sprintf(buf, "%u", c); > + else > + ret += scnprintf(buf+ret, PAGE_SIZE-ret, ",%u", c); 'buf + ret', PAGE_SIZE - ret, Please. > + } > + raw_spin_unlock_irqrestore(&desc->lock, flags); > + > + if (ret) This conditional is pointless. There is always at least one cpu online otherwise you would not be executing this function .... > + ret += scnprintf(buf+ret, PAGE_SIZE-ret, "\n"); And the whole thing can be simplified to: int cpu, irq = desc->irq_data.irq; ssize_t ret = 0; char *p = ""; for_each_online_cpu(cpu) { unsigned int c = kstat_irqs_cpu(irq, cpu); ret += scnprintf(buf + ret , PAGE_SIZE - ret, "%s%u", p, c); p = ","; } ret += scnprintf(buf + ret , PAGE_SIZE - ret, "\n"); return ret; Hmm? > +IRQ_ATTR_RO(per_cpu_count); > + > +static ssize_t chip_name_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj); > + unsigned long flags; > + ssize_t ret = 0; > + > + raw_spin_lock_irqsave(&desc->lock, flags); raw_spin_lock_irq() because this is always called from non interrupt disabled code. No need for flags. > + if (desc->irq_data.chip->name) chip can be NULL ..... > + ret = scnprintf(buf, PAGE_SIZE, "%s\n", > + desc->irq_data.chip->name); If you break the line of the conditional statement, please add a pair of curly braces. > +static ssize_t actions_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj); > + struct irqaction *action; > + unsigned long flags; > + ssize_t ret = 0; > + > + raw_spin_lock_irqsave(&desc->lock, flags); > + for (action = desc->action; action != NULL; action = action->next) { > + if (!ret) > + ret = scnprintf(buf, PAGE_SIZE, "%s", action->name); > + else > + ret += scnprintf(buf+ret, PAGE_SIZE-ret, ",%s", > + action->name); > + } > + raw_spin_unlock_irqrestore(&desc->lock, flags); > + > + if (ret) > + ret += scnprintf(buf+ret, PAGE_SIZE-ret, "\n"); Can be deuglified as well. > @@ -261,6 +410,12 @@ static int alloc_descs(unsigned int start, unsigned int cnt, int node, > goto err; > mutex_lock(&sparse_irq_lock); > irq_insert_desc(start + i, desc); > + if (irq_kobj) { > + if (kobject_add(&desc->kobj, irq_kobj, "%d", start + i)) > + printk(KERN_WARNING > + "Fail to add kobject for irq %d\n", > + start + i); pr_warn please > + } Please split that out into a a helper function. irq_sysfs_add() or such so you avoid that mess in the code and you can reuse that for the init stuff. > +static void irq_kobj_release(struct kobject *kobj) > +{ > + struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj); > + > + /* > + * This irq_desc is statically allocated. Simply zero its kobject > + * rather than kfree it. > + */ > + memset(&desc->kobj, 0, sizeof(desc->kobj)); This will nicely explode when the irq descriptor is handed out again before this function is called. There is no point to make this work for !SPARSE_IRQ. Just ignore it. > +static int __init irq_sysfs_init(void) Again: CONFIG_SYSFS .... > +{ > + int i; > + > + irq_kobj = kobject_create_and_add("irq", kernel_kobj); > + if (!irq_kobj) > + return -ENOMEM; > + > + irq_lock_sparse(); > + for (i = 0; i < nr_irqs; ++i) { for_each_irq_desc() .... > + struct irq_desc *desc = irq_to_desc(i); > + > + if (!desc) > + continue; > + > + if (kobject_add(&desc->kobj, irq_kobj, "%d", i)) > + printk(KERN_WARNING > + "Fail to add kobject for irq %d\n", i); So you managed to copy the same thing 3 times .... Thanks, tglx
Powered by blists - more mailing lists