Re: [RFC 1/6] qed: Add support for hardware offloaded iSCSI.

[Johannes Thumshirn <jthumshirn@suse.de>]

Hi Arun,

On Wed, Oct 19, 2016 at 05:14:59PM -0700, Arun Easi wrote:
> Thanks Johannes for the review, please see my response below.
> 

[...]

> > 
> > Why not introduce a small helper like:
> > static inline bool qed_is_iscsi_personality()
> > {
> > 	return IS_ENABLED(CONFIG_QEDI) && p_hwfn->hw_info.personality ==
> > 		QED_PCI_ISCSI;
> > }
> 
> I think I can remove the IS_ENABLED() check in places like this
> and have the check contained in header file. qed_iscsi_free()
> already is taken care, if I do the same fore qed_ooo*, I think
> the check would just be "p_hwfn->hw_info.personality ==
> QED_PCI_ISCSI", which would keep it consistent with the other
> areas where similar check is done for other protocols.

Sounds good.

> 

[...]

> > 
> > Is there any chance you could factor out above blocks into own functions so
> > you have
> > 
> > 
> > 	if (!GET_FIELD(p_ramrod->iscsi.flags,
> > 		       ISCSI_CONN_OFFLOAD_PARAMS_TCP_ON_CHIP_1B)) {
> > 		qedi_do_stuff_off_chip();
> > 	else 
> > 		qedi_do_stuff_on_chip();
> > 
> 
> This function mostly fills data needed for the firmware interface.
> By having all data necessary for the command
> ISCSI_RAMROD_CMD_ID_OFFLOAD_CONN in this function it is easier to
> refer what is being fed to firmware. If you do not have strong
> objections, I would like to keep it this way.

No strong objections, I just don't think these lengthy blocks are readable,
but I don't want to do too much bikeshedding about it.

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850