netdev - Re: [PATCH 3/4] net: macb: Add hardware PTP support

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] [day] [month] [year] [list]

Message-ID: <20170503094304.GD2340@localhost.localdomain>
Date:   Wed, 3 May 2017 11:43:04 +0200
From:   Richard Cochran <richardcochran@...il.com>
To:     Rafal Ozieblo <rafalo@...ence.com>
Cc:     David Miller <davem@...emloft.net>,
        "nicolas.ferre@...el.com" <nicolas.ferre@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "harinikatakamlinux@...il.com" <harinikatakamlinux@...il.com>,
        "harini.katakam@...inx.com" <harini.katakam@...inx.com>,
        "Andrei.Pistirica@...rochip.com" <Andrei.Pistirica@...rochip.com>
Subject: Re: [PATCH 3/4] net: macb: Add hardware PTP support

On Tue, May 02, 2017 at 01:57:15PM +0000, Rafal Ozieblo wrote:
> > What is the point of this wrapper function anyhow?  Please remove it.
> gem_ptp_gettime() is assigned in ptp_clock_info and it has to have 
> ptp_clock_info pointer as first parameter. gem_tsu_get_time() is used in
> the source code but with macb pointer.
> Do you want me to do something like:
> gem_ptp_gettime(macb->ptp, ts);
> and first would be getting macb pointer from ptp ?
> struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);

Yes.  Unless your sub-function is used in more than one place, then it
is wasteful and confusing to wrap the functionality for no apparent
reason.

> > > +	switch (rq->type) {
> > > +	case PTP_CLK_REQ_EXTTS:	/* Toggle TSU match interrupt */
> > > +		if (on)
> > > +			macb_writel(bp, IER, MACB_BIT(TCI));
> > 
> > No locking to protect IER and IDE?
> There is no need.

But what happens when the PTP_CLK_REQ_EXTTS and PTP_CLK_REQ_PPS ioctls
are called at the same time?

You need to ensure that IDR is consistent.  If the bits are write
only, then you should comment this fact.

> > > +		else
> > > +			macb_writel(bp, IDR, MACB_BIT(TCI));
> > > +		break;
> > > +	case PTP_CLK_REQ_PEROUT: /* Toggle Periodic output */
> > > +		return -EOPNOTSUPP;
> > > +		/* break; */
> > > +	case PTP_CLK_REQ_PPS:	/* Toggle TSU periodic (second)
> > interrupt */
> > > +		if (on)
> > > +			macb_writel(bp, IER, MACB_BIT(SRI));
> > > +		else
> > > +			macb_writel(bp, IDR, MACB_BIT(SRI));
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +	return 0;
> > > +}

Thanks,
Richard