linux-kernel - RE: [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable Register Driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM4PR84MB19276978EB630CF08833E5A488229@DM4PR84MB1927.NAMPRD84.PROD.OUTLOOK.COM>
Date:   Wed, 12 Oct 2022 20:25:00 +0000
From:   "Hawkins, Nick" <nick.hawkins@....com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
CC:     "Verdun, Jean-Marie" <verdun@....com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable
 Register Driver

Greetings Krysztof,

Thanks for the feedback! I have several questions below:

> > +
> > +static ssize_t server_id_show(struct device *dev, struct 
> > +device_attribute *attr, char *buf) {
> > +	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(dev);
> > +	int value_upper;
> > +	int value_lower;
> > +	ssize_t ret;
> > +	u32 trans_offset;
> > +	u32 trans_shift;
> > +
> > +	/* read upper first */
> > +	address_translation(drvdata->server_id.upper[BYTE],
> > +			    &trans_offset,
> > +			    &trans_shift);
> > +	regmap_read(drvdata->plreg_map, trans_offset, &value_upper);
> > +	value_upper = value_upper >> trans_shift;
> > +	value_upper = value_upper & drvdata->server_id.upper[MASK];
> > +
> > +	/* read lower last */
> > +	address_translation(drvdata->server_id.lower[BYTE],
> > +			    &trans_offset,
> > +			    &trans_shift);
> > +	regmap_read(drvdata->plreg_map, trans_offset, &value_lower);
> > +	value_lower = value_lower >> trans_shift;
> > +	value_lower = value_lower & drvdata->server_id.lower[MASK];
> > +
> > +	ret = sprintf(buf, "0x%04x", value_upper | value_lower);
> > +
> > +	return ret;
> > +}
> > +
> > +static DEVICE_ATTR_RO(server_id);

> Missing sysfs documentation.

Can you point me at the proper location / documentation for documenting sysfs? Thanks!

> > +	for (i = 0; i <= MASK; i++) {
> > +		if (of_property_read_u32_index(np, "grp5", i,
> > +					       &drvdata->grp_intr_flags.grp5[i])) {
> > +			dev_err(&pdev->dev,
> > +				"grp5intsflags is missing its 'grp5' property index %d\n", i);
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> > +	np = of_get_child_by_name(pdev->dev.of_node, "pwrbtn");
> > +	if (!np) {
> > +		dev_err(&pdev->dev, "%pOF is missing its 'pwrbtn' node\n", np);
> > +		return -ENODEV;
> > +	}
> > +
> > +	for (i = 0; i <= VALUE; i++) {
> > +		if (of_property_read_u32_index(np, "latch", i, 
> > +&drvdata->pwrbtn.latch[i])) {

> Undocumented properties. NAK.

If each child node of hpe,gxp-plreg were documented with their respective properties would this be acceptable?

Such as:
Fan-1 {
	Compatible="hpe,gxp-plreg-fan";
	id = <0x27>;
	bit = <0x01>;
	fail = <0x24>;
	inst = <0x22>;
};
	

> > +			dev_err(&pdev->dev, "pwrbtn is missing its 'latch' property index %d\n", i);
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> > +	drvdata->gpio_chip = plreg_chip;
> > +	drvdata->gpio_chip.ngpio = 100;
> > +	drvdata->gpio_chip.parent = &pdev->dev;
> > +
> > +	girq = &drvdata->gpio_chip.irq;
> > +	girq->chip = &gxp_gpio_irqchip;
> > +	/* This will let us handle the parent IRQ in the driver */
> > +	girq->parent_handler = NULL;
> > +	girq->num_parents = 0;
> > +	girq->parents = NULL;
> > +	girq->default_type = IRQ_TYPE_NONE;
> > +	girq->handler = handle_edge_irq;
> > +	/* Set up interrupt from PLREG Group 5 Mask */

> Please read Linux coding style. There is a chapter about size of functions. Read it. Actually, read the rest as well... It's ok if
probe() grows above our limit, but this is huge. Really too big.

I will do this and work on breaking up this code into smaller functions.

Thanks,

-Nick Hawkins