RE: [RFC v2 net-next 05/10] qede: Add basic network device support
> > >> > +struct qede_rx_queue {
> > >> > + __le16 *hw_cons_ptr;
> > >>
> > >> The __ variants of constants should be reserved for use in user
> > >> visible API's
> > >
> > > Really? If so, this needs to be fixed not only here but in lots of
> > > places in the series [e.g., entire HW HSI uses __le variants instead of
> > > le].
> > > But why is it so? I.e., I understand that __le16 is defined in the
> > > uapi directory and thus accessible to users, but why the distinction?
> >
> > Because it shows whether the type is something exposed to userspace or not.
> >
> > If there are places where this is done incorrectly in the tree, it is
> > not a legitimate reason for you to do so as well.
>
> Obviously.
> We'll fix all of those for next version.
I've taken a look and I couldn't find reference to 'le16' anywhere under
drivers/net/ethernet/. And 'le16' is actually a fs/ntfs/types.h definition.
What am I missing here?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC v2 net-next 05/10] qede: Add basic network device support
> >> > +struct qede_rx_queue {
> >> > +__le16 *hw_cons_ptr;
> >>
> >> The __ variants of constants should be reserved for use in user
> >> visible API's
> >
> > Really? If so, this needs to be fixed not only here but in lots of
> > places in the series [e.g., entire HW HSI uses __le variants instead of le].
> > But why is it so? I.e., I understand that __le16 is defined in the
> > uapi directory and thus accessible to users, but why the distinction?
>
> Because it shows whether the type is something exposed to userspace or not.
>
> If there are places where this is done incorrectly in the tree, it is not a
> legitimate
> reason for you to do so as well.
Obviously.
We'll fix all of those for next version.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 net-next 05/10] qede: Add basic network device support
From: Yuval Mintz
Date: Fri, 25 Sep 2015 05:43:06 +
>> > +struct qede_rx_queue {
>> > + __le16 *hw_cons_ptr;
>>
>> The __ variants of constants should be reserved for use in user visible API's
>
> Really? If so, this needs to be fixed not only here but in lots of places
> in the series [e.g., entire HW HSI uses __le variants instead of le].
> But why is it so? I.e., I understand that __le16 is defined in the uapi
> directory and
> thus accessible to users, but why the distinction?
Because it shows whether the type is something exposed to userspace or not.
If there are places where this is done incorrectly in the tree, it is not
a legitimate reason for you to do so as well.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC v2 net-next 05/10] qede: Add basic network device support
> > > +#define QEDE_NAPI_WEIGHT (NAPI_POLL_WEIGHT)
> > Why not just use existing constant rather than walpapering?
No reason; We'll remove it.
> > > +#define U64_LO(x)((u32)(((u64)(x)) & 0x))
> > > +#define U64_HI(x)((u32)(((u64)(x)) >> 32))
> > > +#define HILO_U64(hi, lo) u64)(hi)) << 32) + (lo))
> >
> > Why reinvent upper32 and lower32?
Ignorance? And I assume you meant {upper, lower}_32_bits.
Is there also a standard macro doing what HILO_U64 does?
> > +struct qede_rx_queue {
> > + __le16 *hw_cons_ptr;
>
> The __ variants of constants should be reserved for use in user visible API's
Really? If so, this needs to be fixed not only here but in lots of places
in the series [e.g., entire HW HSI uses __le variants instead of le].
But why is it so? I.e., I understand that __le16 is defined in the uapi
directory and
thus accessible to users, but why the distinction?
[I'm looking at bnx2x_hsi.h for reference which is full with the __le variants;
Not saying that's a good thing, though]
> > > + int rx_buf_size;
> >
> > Shouldn't rx_buf_size be unsigned?
Yes.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 net-next 05/10] qede: Add basic network device support
On Thu, 17 Sep 2015 17:23:54 +0300
Yuval Mintz wrote:
> +#define QEDE_NAPI_WEIGHT (NAPI_POLL_WEIGHT)
> +
Why not just use existing constant rather than walpapering?
> +
> +#define U64_LO(x)((u32)(((u64)(x)) & 0x))
> +#define U64_HI(x)((u32)(((u64)(x)) >> 32))
> +#define HILO_U64(hi, lo) u64)(hi)) << 32) + (lo))
Why reinvent upper32 and lower32?
> +struct qede_rx_queue {
> + __le16 *hw_cons_ptr;
The __ variants of constants should be reserved for use in user visible
API's
> + struct sw_rx_data *sw_rx_ring;
> + u16 sw_rx_cons;
> + u16 sw_rx_prod;
> + struct qed_chainrx_bd_ring;
> + struct qed_chainrx_comp_ring;
> + void __iomem*hw_rxq_prod_addr;
> +
> + int rx_buf_size;
Shouldn't rx_buf_size be unsigned?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
