Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Coiby Xu <coiby.xu@gmail.com>
Date:   Mon, 12 Oct 2020 19:51:14 +0800
To:     Benjamin Poirier <benjamin.poirier@gmail.com>
Cc:     devel@driverdev.osuosl.org, Shung-Hsi Yu <shung-hsi.yu@suse.com>,
        Manish Chopra <manishc@marvell.com>,
        "supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER" 
        <GR-Linux-NIC-Dev@marvell.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "open list:QLOGIC QLGE 10Gb ETHERNET DRIVER" <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health
 reporter
Message-ID: <20201012115114.lyh33rvmm4rt7mej@Rk>
References: <20201008115808.91850-1-coiby.xu@gmail.com>
 <20201008115808.91850-3-coiby.xu@gmail.com>
 <20201010074809.GB14495@f3>
 <20201010100258.px2go6nugsfbwoq7@Rk>
 <20201010132230.GA17351@f3>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20201010132230.GA17351@f3>
Precedence: bulk

On Sat, Oct 10, 2020 at 10:22:30PM +0900, Benjamin Poirier wrote:
>On 2020-10-10 18:02 +0800, Coiby Xu wrote:
>[...]
>> > > +	do {                                                           \
>> > > +		err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
>> > > +		if (err) {					       \
>> > > +			kvfree(dump);                                  \
>> > > +			return err;				       \
>> > > +		}                                                      \
>> > > +	} while (0)
>> > > +
>> > > +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
>> > > +				  struct devlink_fmsg *fmsg, void *priv_ctx,
>> > > +				  struct netlink_ext_ack *extack)
>> > > +{
>> > > +	int err = 0;
>> > > +
>> > > +	struct qlge_devlink *dev = devlink_health_reporter_priv(reporter);
>> >
>> > Please name this variable ql_devlink, like in qlge_probe().
>>
>> I happened to find the following text in drivers/staging/qlge/TODO
>> > * in terms of namespace, the driver uses either qlge_, ql_ (used by
>> >  other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with
>> >  clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_"
>> >  prefix.
>
>This comment applies to global identifiers, not local variables.

Thank you for the explanation! Are you suggesting we should choose
different naming styles so we better tell global identifiers from local
variables?
>
>>
>> So I will adopt qlge_ instead. Besides I prefer qlge_dl to ql_devlink.
>
>Up to you but personally, I think ql_devlink is better. In any case,
>"dev" is too general and often used for struct net_device pointers
>instead.

Thank you for the suggestion. Another reason to use qlge_dl is many
other network drivers supporting devlink interface also adopt this kind
of style.

--
Best regards,
Coiby