Yi Zou wrote:
> These helpers are used by LLD and are removed in a recent patch in the
> fcoe-features tree. This patch adds them back to libfcoe.h
>
> Signed-off-by: Yi Zou <[email protected]>
> ---
>
> include/scsi/libfcoe.h | 43 +++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 43 insertions(+), 0 deletions(-)
>
> diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
> index 666cc13..318f30a 100644
> --- a/include/scsi/libfcoe.h
> +++ b/include/scsi/libfcoe.h
> @@ -161,4 +161,47 @@ int fcoe_ctlr_recv_flogi(struct fcoe_ctlr *, struct
> fc_frame *fp, u8 *sa);
> u64 fcoe_wwn_from_mac(unsigned char mac[], unsigned int, unsigned int);
> int fcoe_libfc_config(struct fc_lport *, struct libfc_function_template *);
>
> +/* fcoe related skb helpers */
> +static inline struct fcoe_hdr *skb_fcoe_header(const struct sk_buff *skb)
> +{
> + return (struct fcoe_hdr *)skb_network_header(skb);
> +}
Let's get rid of this routine anyway and just use the cast. It's clearer.
Eventually, skb_network_header() should be modified to return void * so the
cast would be unnecessary.
> +
> +static inline int skb_fcoe_offset(const struct sk_buff *skb)
> +{
> + return skb_network_offset(skb);
> +}
This function just does a name change, right? Let's not hide the fact that
the fcoe header is the network header.
> +static inline struct fc_frame_header *skb_fc_header(const struct sk_buff
> *skb)
> +{
> + return (struct fc_frame_header *)skb_transport_header(skb);
> +}
Similarly, let's not hide the cast ... just use skb_transport_header directly.
> +
> +static inline int skb_fc_offset(const struct sk_buff *skb)
> +{
> + return skb_transport_offset(skb);
> +}
See skb_fcoe_offset() comment.
> +static inline void skb_reset_fc_header(struct sk_buff *skb)
> +{
> + skb_reset_network_header(skb);
> + skb_set_transport_header(skb, skb_network_offset(skb) +
> + sizeof(struct fcoe_hdr));
> +}
Let's just do this everywhere (only one or two places I bet)
that this is called.
> +static inline bool skb_fc_is_roff(const struct sk_buff *skb)
> +{
> + return skb_fc_header(skb)->fh_f_ctl[2] & FC_FC_REL_OFF;
> +}
Let's make this a function that just tests bits in a "be24" array.
That's more useful to drivers which don't have an skb.
Here's a general (untested) function that masks a be24 value with a host-ordered
mask. When the mask is a constant, the optimizer will not generate code for
the if-statements, just the code for the bodies of the true if-statements.
static inline u32 be24_mask(u8 *val, u32 mask)
{
if (mask >> 16)
mask &= val[0] << 16;
if ((mask >> 8) & 0xff)
mask &= val[1] << 8;
if (mask & 0xff)
mask &= val[2];
return mask;
}
So, if we use this as: be24_mask(fh->fh_f_ctl, FC_FC_REL_OFF), the
compiler will just generate the equivalent of:
return FC_FC_REL_OFF & fh->fh_f_ctl[2];
I would put that function in libfc.h. Note that ntoh24 is the
same as be24_mask(p, 0xffffff).
The above function is a better way of testing any f_ctl bit when
the f_ctl is in network order.
Then instead of using skb_fc_is_roff(), do the more readable, perhaps:
if (be24_mask(fh->fh_f_ctl, FH_FH_REL_OFF))
...
Instead of skb_fc_is_roff and the following two functions, let's set
a local variable to the fc_frame_header pointer and then access
it directly. Otherwise, we could be hiding extra skb offset
and pointer accesses that re-compute the fc_frame_header pointer
unnecessarily. A smart compiler might be able to optimize these
out, but it seems clearer in source code to just do them once.
> +static inline u16 skb_fc_oxid(const struct sk_buff *skb)
> +{
> + return be16_to_cpu(skb_fc_header(skb)->fh_ox_id);
> +}
> +
> +static inline u16 skb_fc_rxid(const struct sk_buff *skb)
> +{
> + return be16_to_cpu(skb_fc_header(skb)->fh_rx_id);
> +}
> +
> #endif /* _LIBFCOE_H */
Hiding the be16_to_cpu() is undesirable since it could
be cheaper to access the un-swapped field directly for comparisons.
If now isn't the time to make the adjustments I'm suggesting,
let's keep these wrappers in the LLD and migrate to not using them at all.
Regards,
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
