netdev - Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT

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: <20190123131502.GA26413@lunn.ch>
Date:   Wed, 23 Jan 2019 14:15:02 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Igor Russkikh <Igor.Russkikh@...antia.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Nikita Danilov <Nikita.Danilov@...antia.com>
Subject: Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR
 construct

On Wed, Jan 23, 2019 at 09:49:25AM +0000, Igor Russkikh wrote:
> 
> > 
> > Hi Igor
> > 
> > 	 err = readx_poll_timeout(hw_atl_itr_res_irq_get, self, alt_itr_res,
> > 	       		          alt_itr_res == 0, 10, 1000);
> > 
> > The advantage of using readx_poll_timeout is that it is used by lots
> > of other drivers and works. It is much better to use core
> > infrastructure, then build your own.
> 
> Hi Andrew, agreed, but driver have more incompatible places with constructs like
> 
> 	AQ_HW_WAIT_FOR(orig_stats_val !=
> 		       (aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR) &
> 				       BIT(CAPS_HI_STATISTICS)),
> 		       1U, 10000U);

You can define a little helper:

static u32 aq_hw_read_mpi_state2_addr(struct aq_hw_s *self)
{
	return aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR);
}

Then

	readx_poll_timeout(u32 aq_hw_read_mpi_state2_addr, self,
			   stats_val, orig_stats_val != stats_val & BIT(CAPS_HI_STATISTICS));

Given you have the comment:

        /* Wait FW to report back */

You think the current code is not very readable. So you could actually
have:

static int sq_fw2_update_stats_fw_wait(aq_hw_s *self, u32 orig_stats_val)
{
	u32 stats_val;

	return readx_poll_timeout(u32 aq_hw_read_mpi_state2_addr, self,
			           stats_val,
				   orig_stats_val != stats_val & BIT(CAPS_HI_STATISTICS),
				   1, 1000);
}

You see this sort of construct in quite a lot of drivers.

> Found duplicating readl_poll_timeout declaration here:
> https://elixir.bootlin.com/linux/latest/source/drivers/phy/qualcomm/phy-qcom-ufs-i.h#L27
> Not sure what's the reason, but may worth cleaning it up.

Thanks.

	Andrew