netdev - Re: [PATCH V6 net-next 11/11] ptp: Add a driver for InES time stamping IP core.

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: <20191217153345.GH17965@lunn.ch>
Date:   Tue, 17 Dec 2019 16:33:45 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Richard Cochran <richardcochran@...il.com>
Cc:     netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
        devicetree@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jacob Keller <jacob.e.keller@...el.com>,
        Mark Rutland <mark.rutland@....com>,
        Miroslav Lichvar <mlichvar@...hat.com>,
        Murali Karicheri <m-karicheri2@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Wingman Kwok <w-kwok2@...com>
Subject: Re: [PATCH V6 net-next 11/11] ptp: Add a driver for InES time
 stamping IP core.

> +static int ines_clock_init(struct ines_clock *clock, struct device_node *node,
> +			   void __iomem *addr)
> +{
> +	unsigned long port_addr;
> +	struct ines_port *port;
> +	int i, j;
> +
> +	INIT_LIST_HEAD(&clock->list);
> +	clock->node = node;
> +	clock->base = addr;
> +	clock->regs = clock->base;
> +
> +	for (i = 0; i < INES_N_PORTS; i++) {
> +		port = &clock->port[i];
> +		port_addr = (unsigned long) clock->base +
> +			INES_PORT_OFFSET + i * INES_PORT_SIZE;
> +		port->regs = (struct ines_port_registers *) port_addr;
> +		port->clock = clock;
> +		port->index = i;
> +		INIT_DELAYED_WORK(&port->ts_work, ines_txtstamp_work);
> +		spin_lock_init(&port->lock);
> +		INIT_LIST_HEAD(&port->events);
> +		INIT_LIST_HEAD(&port->pool);
> +		for (j = 0; j < INES_MAX_EVENTS; j++)
> +			list_add(&port->pool_data[j].list, &port->pool);
> +	}
> +
> +	ines_write32(clock, 0xBEEF, test);
> +	ines_write32(clock, 0xBEEF, test2);
> +
> +	pr_debug("ID      0x%x\n", ines_read32(clock, id));
> +	pr_debug("TEST    0x%x\n", ines_read32(clock, test));
> +	pr_debug("VERSION 0x%x\n", ines_read32(clock, version));
> +	pr_debug("TEST2   0x%x\n", ines_read32(clock, test2));
> +

Hi Richard

Using pr_ functions is frowned upon. You have a device, since this is
a platform driver, please use dev_debug() etc.

> +	for (i = 0; i < INES_N_PORTS; i++) {
> +		port = &clock->port[i];
> +		ines_write32(port, PORT_CONF, port_conf);
> +	}
> +
> +	return 0;
> +}
> +
> +static void ines_dump_ts(char *label, struct ines_timestamp *ts)
> +{
> +#ifdef DEBUG
> +	pr_err("%s timestamp, tag=0x%04hx t=%llu.%9llu c=0x%llx p=%hu s=%hu\n",
> +	       label, ts->tag, ts->sec, ts->nsec,
> +	       ts->clkid, ts->portnum, ts->seqid);
> +#endif

DEBUG using pr_err() is a bit unusual. dev_dbg() would be normal.

> +static int ines_ptp_ctrl_probe(struct platform_device *pld)
> +{
> +	struct ines_clock *clock;
> +	struct resource *res;
> +	void __iomem *addr;
> +	int err = 0;
> +
> +	res = platform_get_resource(pld, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pld->dev, "missing memory resource\n");
> +		return -EINVAL;
> +	}
> +	addr = devm_ioremap_resource(&pld->dev, res);
> +	if (IS_ERR(addr)) {
> +		err = PTR_ERR(addr);
> +		goto out;
> +	}
> +	clock = kzalloc(sizeof(*clock), GFP_KERNEL);

Do the different memory life cycles allow devm_ to be used here?