Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

2012-03-21 Thread Jason Gunthorpe 
On Wed, Mar 21, 2012 at 12:33:00PM -0700, Roland Dreier wrote:
> On Wed, Mar 21, 2012 at 12:04 PM,   wrote:
> >> > +/* mailbox cmd response */
> >> > +struct ocrdma_mbx_rsp {
> >> > + ? ? ? u32 subsys_op;
> >> > + ? ? ? u32 status;
> >> > + ? ? ? u32 rsp_len;
> >> > + ? ? ? u32 add_rsp_len;
> >> > +} __packed;
> 
> >> ...similar comments about only using __packed where you really need it...
> 
> > This pack is required as it is shared with hardware and need to be
> > of 16 bytes for 32 and 64 bit architecture. Do not wanted to take
> > risk of different compiler versions. So keeping it packed.
> 
> I really think if you can't trust your compiler to lay this
> structure out properly, you have a lot of bigger problems.  But
> whatever, it's not a big deal.

Doesn't packed penalize all access to the structure on some
architectures, eg sparc?

A static assert is a better choice than packed...

BUILD_BUG_ON(sizeof(ocrdma_mbx_rsp) != 16);

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

2012-03-21 Thread Roland Dreier 
On Wed, Mar 21, 2012 at 12:04 PM,   wrote:
>> > +/* mailbox cmd response */
>> > +struct ocrdma_mbx_rsp {
>> > +       u32 subsys_op;
>> > +       u32 status;
>> > +       u32 rsp_len;
>> > +       u32 add_rsp_len;
>> > +} __packed;

>> ...similar comments about only using __packed where you really need it...

> This pack is required as it is shared with hardware and need to be of 16 
> bytes for 32 and 64 bit architecture. Do not wanted to take risk of different 
> compiler versions. So keeping it packed.

I really think if you can't trust your compiler to lay this structure
out properly,
you have a lot of bigger problems.  But whatever, it's not a big deal.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

2012-03-21 Thread Parav.Pandit 


> -Original Message-
> From: Roland Dreier [mailto:rol...@purestorage.com]
> Sent: Wednesday, March 21, 2012 9:56 PM
> To: Pandit, Parav
> Cc: linux-rdma@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> > +/* mailbox cmd response */
> > +struct ocrdma_mbx_rsp {
> > +       u32 subsys_op;
> > +       u32 status;
> > +       u32 rsp_len;
> > +       u32 add_rsp_len;
> > +} __packed;
> 
> ...similar comments about only using __packed where you really need it...
This pack is required as it is shared with hardware and need to be of 16 bytes 
for 32 and 64 bit architecture. Do not wanted to take risk of different 
compiler versions. So keeping it packed.

> 
> > +#define is_cqe_valid(cq, cqe) \
> > +       (((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_VALID)\
> > +       == cq->phase) ? 1 : 0)
> > +#define is_cqe_for_sq(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_QTYPE) ?
> > +0 : 1) #define is_cqe_for_rq(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_QTYPE) ?
> > +1 : 0) #define is_cqe_invalidated(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) &
> > +OCRDMA_CQE_INVALIDATE) ? \
> > +       1 : 0)
> > +#define is_cqe_imm(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_IMM) ? 1
> > +: 0) #define is_cqe_wr_imm(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) &
> > +OCRDMA_CQE_WRITE_IMM) ? 1 : 0)
> 
> ...similar comment about using readable typesafe inline functions instead of
> macros...

Yes, I'll change to inline function.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

2012-03-21 Thread David Laight 
 
> > +#define is_cqe_wr_imm(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_WRITE_IMM) ? 1 
> > : 0)
> 
> ...similar comment about using readable typesafe inline functions
> instead of macros...

and if you are using #defines, you need to enclose every reference
to the parameters in ().

David


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

2012-03-21 Thread Roland Dreier 
> +/* mailbox cmd response */
> +struct ocrdma_mbx_rsp {
> +       u32 subsys_op;
> +       u32 status;
> +       u32 rsp_len;
> +       u32 add_rsp_len;
> +} __packed;

...similar comments about only using __packed where you really need it...

> +#define is_cqe_valid(cq, cqe) \
> +       (((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_VALID)\
> +       == cq->phase) ? 1 : 0)
> +#define is_cqe_for_sq(cqe) \
> +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_QTYPE) ? 0 : 1)
> +#define is_cqe_for_rq(cqe) \
> +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_QTYPE) ? 1 : 0)
> +#define is_cqe_invalidated(cqe) \
> +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_INVALIDATE) ? \
> +       1 : 0)
> +#define is_cqe_imm(cqe) \
> +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_IMM) ? 1 : 0)
> +#define is_cqe_wr_imm(cqe) \
> +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_WRITE_IMM) ? 1 : 
> 0)

...similar comment about using readable typesafe inline functions
instead of macros...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html