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
[email protected] +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
