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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161020070901.52tm2d255dquocho@linux-x5ow.site>
Date:   Thu, 20 Oct 2016 09:09:01 +0200
From:   Johannes Thumshirn <jthumshirn@...e.de>
To:     Arun Easi <arun.easi@...ium.com>
Cc:     manish.rangankar@...ium.com, lduncan@...e.com, cleech@...hat.com,
        martin.petersen@...cle.com, jejb@...ux.vnet.ibm.com,
        linux-scsi@...r.kernel.org, netdev@...r.kernel.org,
        Yuval.Mintz@...ium.com, QLogic-Storage-Upstream@...ium.com,
        Yuval Mintz <Yuval.Mintz@...gic.com>
Subject: Re: [RFC 1/6] qed: Add support for hardware offloaded iSCSI.

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@...e.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