linux-kernel - Re: [PATCH net-next 10/13] qed: add support for new port modes

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] [day] [month] [year] [list]

Message-ID: <20200717192633.181-1-alobakin@marvell.com>
Date:   Fri, 17 Jul 2020 22:26:33 +0300
From:   Alexander Lobakin <alobakin@...vell.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Alexander Lobakin <alobakin@...vell.com>,
        Igor Russkikh <irusskikh@...vell.com>,
        <linux-scsi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Michal Kalderon <mkalderon@...vell.com>,
        Ariel Elior <aelior@...vell.com>,
        Denis Bolotin <dbolotin@...vell.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        GR-everest-linux-l2 <GR-everest-linux-l2@...vell.com>,
        <QLogic-Storage-Upstream@...vell.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 10/13] qed: add support for new port modes

Date: Fri, 17 Jul 2020 11:31:55 -0700
From: Jakub Kicinski <kuba@...nel.org>

> On Fri, 17 Jul 2020 13:49:33 +0300 Igor Russkikh wrote:
>>> ----------------------------------------------------------------------
>>> On Thu, 16 Jul 2020 14:54:43 +0300 Alexander Lobakin wrote:  
>>>> These ports ship on new boards revisions and are supported by newer
>>>> firmware versions.
>>>>
>>>> Signed-off-by: Alexander Lobakin <alobakin@...vell.com>
>>>> Signed-off-by: Igor Russkikh <irusskikh@...vell.com>  
>>> 
>>> What is the driver actually doing with them, tho?
>>> 
>>> Looks like you translate some firmware specific field to a driver
>>> specific field, but I can't figure out what part of the code cares
>>> about hw_info.port_mode  
>> 
>> Hi Jakub,
>> 
>> You are right, this info is never used/reported.
>> 
>> Alexander is extending already existing non used field with new values from
>> our latest hardware revisions.
>> 
>> I thought devlink info could be a good place to output such kind of information.
>> 
>> Thats basically a layout of *Physical* ports on device - quite useful info I
>> think.
>> 
>> Important thing is these ports may not be directly mapped to PCI PFs. So
>> reading `ethtool eth*` may not explain you the real device capabilities.
>> 
>> Do you think it makes sense adding such info to `devlink info` then?
>
> Devlink port has information about physical port, which don't have to
> map 1:1 to netdevs. It also has lanes and port splitting which you may
> want to report.
>
>
> For now please make sure to not include any dead code in your
> submissions (register defines etc. may be okay), perhaps try:

I think it would be better to drop this struct member at all since it's
really not used anywhere. There'll be no problem to add it back anytime
we might need it, but for now it seems unreasonable to keep it "just in
case".

Got it, thanks, will send v2 soon.

> diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
> index d929556247a5..4bad836d0f74 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
> @@ -4026,6 +4026,21 @@ static int qed_hw_get_nvm_info(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
>  	case NVM_CFG1_GLOB_NETWORK_PORT_MODE_4X25G:
>  		p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_4X25G;
>  		break;
> +	case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X50G_R1:
> +	case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_4X50G_R1:
> +	case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R2:
> +	case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X100G_R2:
> +	case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R4:
> +		/* TODO: set port_mode when it's actually used */
> +		break;
>  	default:
>  		DP_NOTICE(p_hwfn, "Unknown port mode in 0x%08x\n", core_cfg);
>  		break;
>
> And see if it will pass the muster.
>
> Dead code makes it harder to review the patches.