Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 12 Nov 2019 14:50:45 +0100
From:   Simon Horman <simon.horman@netronome.com>
To:     Po Liu <po.liu@nxp.com>
Cc:     Claudiu Manoil <claudiu.manoil@nxp.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "vinicius.gomes@intel.com" <vinicius.gomes@intel.com>,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        Alexandru Marginean <alexandru.marginean@nxp.com>,
        Xiaoliang Yang <xiaoliang.yang_1@nxp.com>,
        Roy Zang <roy.zang@nxp.com>, Mingkai Hu <mingkai.hu@nxp.com>,
        Jerry Huang <jerry.huang@nxp.com>, Leo Li <leoyang.li@nxp.com>
Subject: Re: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware
 Scheduler via tc-taprio offload
Message-ID: <20191112135045.5qaau7kqdxrrpqo4@netronome.com>
References: <20191111042715.13444-1-Po.Liu@nxp.com>
 <20191112094128.mbfil74gfdnkxigh@netronome.com>
 <VE1PR04MB6496CE5A0DA25D7AF9FD666492770@VE1PR04MB6496.eurprd04.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <VE1PR04MB6496CE5A0DA25D7AF9FD666492770@VE1PR04MB6496.eurprd04.prod.outlook.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 12, 2019 at 11:19:43AM +0000, Po Liu wrote:

...

> > > +/* class 5, command 0 */
> > > +struct tgs_gcl_conf {
> > > +     u8      atc;    /* init gate value */
> > > +     u8      res[7];
> > > +     union {
> > > +             struct {
> > > +                     u8      res1[4];
> > > +                     __le16  acl_len;
> > 
> > Given that u* types are used in this structure I think le16 would be more
> > appropriate than __le16.
>  
> Here keep the same code style of this .h file. I think it is better to have another patch to fix them all. Do you agree?
> 
> > 

> > > +                     u8      res2[2];
> > > +             };
> > > +             struct {
> > > +                     u32 cctl;
> > > +                     u32 ccth;
> > > +             };
> > 
> > I'm a little surprised to see host endian values in a structure that appears to be
> > written to hardware. Is this intentional?
> 
> Will remove.

If the HW defines these fields then I think its fine to leave them,
though with the correct byte-order.

I was more asking if it is intentional that the value for these
fields, when sent to the HW, is always zero in the context of this
patch-set. Likewise elsewhere.

...

> > > +
> > > +     gcl_data->ct = cpu_to_le32(admin_conf->cycle_time);
> > > +     gcl_data->cte = cpu_to_le32(admin_conf->cycle_time_extension);
> > > +
> > > +     for (i = 0; i < gcl_len; i++) {
> > > +             struct tc_taprio_sched_entry *temp_entry;
> > > +             struct gce *temp_gce = gce + i;
> > > +
> > > +             temp_entry = &admin_conf->entries[i];
> > > +
> > > +             temp_gce->gate = cpu_to_le32(temp_entry->gate_mask);
> > 
> >         Gate is a u8 followed by 3 reserved bytes.
> >         Perhaps there needs to be some bounds checking on
> >         the value stored there given that the source is 32bits wide.
> > 
> >         Also, its not clear to me that the above logic, which I assume
> >         takes the last significant byte of a 32bit value, works on
> >         big endian systems as the 32bit value is always little endian.
> 
> temp_entry->gate_mask is 32bit for wide possible input. Here change to hardware set 8bit wide.
> Can it just be like:
> 	temp_gce->gate = (u8) temp_entry->gate_mask;

I think that would be better.
Perhaps its best to also mask out the unwanted bits.

...