linux-kernel - RE: [V5, 2/6] fsl/fman: Add FMan 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM2PR03MB509C1B1A5272C9D0A9F8295E6200@DM2PR03MB509.namprd03.prod.outlook.com>
Date:	Thu, 29 Oct 2015 15:22:23 +0000
From:	Liberman Igal <Igal.Liberman@...escale.com>
To:	Scott Wood <scottwood@...escale.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Madalin-Cristian Bucur <madalin.bucur@...escale.com>
Subject: RE: [V5, 2/6] fsl/fman: Add FMan support


Regards,
Igal Liberman

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, October 28, 2015 11:31 PM
> To: Liberman Igal-B31950 <Igal.Liberman@...escale.com>
> Cc: netdev@...r.kernel.org; linuxppc-dev@...ts.ozlabs.org; linux-
> kernel@...r.kernel.org; Bucur Madalin-Cristian-B32716
> <madalin.bucur@...escale.com>
> Subject: Re: [V5, 2/6] fsl/fman: Add FMan support
> 
> On Tue, 2015-10-27 at 11:32 -0500, Liberman Igal-B31950 wrote:
> 
> > > > +
> > > > +struct device *fman_get_device(struct fman *fman) {  return
> > > > +fman->dev; }
> > >
> > > Is this really necessary?
> > >
> >
> > Fman port needs fman->dev, fman structure is opaque, so yes, it's needed.
> 
> Why is opacity being maintained from one part of the fman driver to
> another?
> Isn't this the sort of excessive layering that was complained about?
> 
> 

It's not really layering.
Fman Port uses Fman resources, it's not completely standalone. 

> > > > +         /* In B4 rev 2.0 (and above) the MURAM size is 512KB.
> > > > +          * Check the SVR and update MURAM size if required.
> > > > +          */
> > > > +         u32 svr;
> > > > +
> > > > +         svr = mfspr(SPRN_SVR);
> > > > +
> > > > +         if ((SVR_SOC_VER(svr) == SVR_B4860) && (SVR_MAJ(svr) >=
> > > 2))
> > > > +                 fman->dts_params.muram_size = 0x80000; }
> > >
> > > Why wasn't the MURAM size described in the device tree, as it was
> > > with CPM/QE?
> > >
> >
> > MURAM size described by the device-tree.
> > In B4860 rev 2.0 (and above) MURAM size is bigger.
> > This is workaround, in order to have the same device tree for all
> > B4860 revisions.
> 
> We don't support b4860 prior to rev 2.0 (due to e6500 core errata) so this is
> irrelevant.  Fix the device tree.
> 

OK, thanks.
I'll submit a new device tree patch (on top of the existing patches, which are awaiting upstream) and remove this code from Fman. 

> > > > +
> > > > + of_node_put(muram_node);
> > > > + of_node_put(fm_node);
> > > > +
> > > > + err = devm_request_irq(&of_dev->dev, irq, fman_irq,
> > > > +                        IRQF_NO_SUSPEND, "fman", fman); if (err <
> > > > + 0) {
> > > > +         pr_err("Error: allocating irq %d (error = %d)\n", irq, err);
> > > > +         goto fman_free;
> > > > + }
> > >
> > > Why IRQF_NO_SUSPEND?
> > >
> >
> > It shouldn't be IRQF_NO_SUSPEND for now, removed.
> 
> Why just "for now"?
> 

Unsuccessful wording, sorry.

> -Scott