Re: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

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

...

quoted

quoted

+/* 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?

quoted

quoted

quoted

+                     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.

...

quoted

quoted

+
+     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.

...