Date:   Fri, 17 Mar 2023 19:06:36 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     "Mukunda,Vijendar" <vijendar.mukunda@amd.com>
Cc:     alsa-devel@alsa-project.org, pierre-louis.bossart@linux.intel.com,
        Basavaraj.Hiregoudar@amd.com, Sunil-kumar.Dommati@amd.com,
        Mario.Limonciello@amd.com, amadeuszx.slawinski@linux.intel.com,
        Mastan.Katragadda@amd.com, Arungopal.kondaveeti@amd.com,
        claudiu.beznea@microchip.com,
        Bard Liao <yung-chuan.liao@linux.intel.com>,
        Sanyog Kale <sanyog.r.kale@intel.com>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V6 5/8] soundwire: amd: add SoundWire manager interrupt
 handling
Message-ID: <ZBRs5LAXRQ2S1Htc@matsya>
References: <20230307133135.545952-1-Vijendar.Mukunda@amd.com>
 <20230307133135.545952-6-Vijendar.Mukunda@amd.com>
 <ZBGYubOYyu7E8ueo@matsya>
 <3aa704b0-1142-8afe-b0f1-28bd2a254cc2@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3aa704b0-1142-8afe-b0f1-28bd2a254cc2@amd.com>
Precedence: bulk

On 16-03-23, 22:34, Mukunda,Vijendar wrote:
> On 15/03/23 15:36, Vinod Koul wrote:
> > On 07-03-23, 19:01, Vijendar Mukunda wrote:

> >> +static void amd_sdw_update_slave_status_work(struct work_struct *work)
> >> +{
> >> +	struct amd_sdw_manager *amd_manager =
> >> +		container_of(work, struct amd_sdw_manager, amd_sdw_work);
> >> +	int retry_count = 0;
> >> +
> >> +	if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
> >> +		acp_reg_writel(0, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
> >> +		acp_reg_writel(0, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
> >> +	}
> >> +
> >> +update_status:
> >> +	sdw_handle_slave_status(&amd_manager->bus, amd_manager->status);
> >> +	/*
> >> +	 * During the peripheral enumeration sequence, the SoundWire manager interrupts
> >> +	 * are masked. Once the device number programming is done for all peripherals,
> >> +	 * interrupts will be unmasked. Read the peripheral device status from ping command
> >> +	 * and process the response. This sequence will ensure all peripheral devices enumerated
> >> +	 * and initialized properly.
> >> +	 */
> >> +	if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
> >> +		if (retry_count++ < SDW_MAX_DEVICES) {
> >> +			acp_reg_writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio +
> >> +				       ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
> >> +			acp_reg_writel(AMD_SDW_IRQ_MASK_8TO11,
> >> +				       amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
> >> +			amd_sdw_read_and_process_ping_status(amd_manager);
> >> +			goto update_status;
> > goto are mostly used for error handling, i dont thing case here deserves
> > a goto, can you please change this...
> I agree. goto statements will be used mostly for error handling.
> But this is a different scenario. We have used goto statement to call sdw_handle_slave_status()
> from if statement to make sure all peripheral devices are enumerated and initialized properly.
> Please let us know if you are expecting code to be modified as mentioned below.
> 
> sdw_handle_slave_status(&amd_manager->bus, amd_manager->status);
> 
> if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
> 	acp_reg_writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio +
> 		       ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
> 	acp_reg_writel(AMD_SDW_IRQ_MASK_8TO11,
> 		       amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
> 	amd_sdw_read_and_process_ping_status(amd_manager);
> 	sdw_handle_slave_status(&amd_manager->bus, amd_manager->status);
> }
> 
> We have to check any race conditions occurs or not if we implement code as mentioned
> above.

what race are you talking about

> IMHO, it is still good to go with goto statement implementation.

Since you keep checking, essentially this seems to be a loop?

-- 
~Vinod