linux-kernel - Re: [PATCH 5/6] firewire: core; eliminate pick

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: <20250918234620.GA127993@workstation.local>
Date: Fri, 19 Sep 2025 08:46:20 +0900
From: Takashi Sakamoto <o-takashi@...amocchi.jp>
To: linux1394-devel@...ts.sourceforge.net
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] firewire: core; eliminate pick_me goto label

Hi,

On Fri, Sep 19, 2025 at 08:08:56AM +0900, Takashi Sakamoto wrote:
> This commit uses condition statements instead of pick_me goto label.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@...amocchi.jp>
> ---
>  drivers/firewire/core-card.c | 102 ++++++++++++++++++-----------------
>  1 file changed, 52 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
> index 6268b595d4fa..b766ce5fdea4 100644
> --- a/drivers/firewire/core-card.c
> +++ b/drivers/firewire/core-card.c
> @@ -388,6 +388,7 @@ static void bm_work(struct work_struct *work)
>  	int root_id, new_root_id, irm_id, local_id;
>  	int expected_gap_count, generation;
>  	bool do_reset = false;
> +	bool stand_for_root;
>  
>  	if (card->local_node == NULL)
>  		return;
> @@ -408,11 +409,11 @@ static void bm_work(struct work_struct *work)
>  			fw_schedule_bm_work(card, msecs_to_jiffies(125));
>  			return;
>  		case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF:
> -			new_root_id = local_id;
> -			goto pick_me;
> +			stand_for_root = true;
> +			break;
>  		case BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY:
> -			new_root_id = local_id;
> -			goto pick_me;
> +			stand_for_root = true;
> +			break;
>  		case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION:
>  			// BM work has been rescheduled.
>  			return;
> @@ -423,8 +424,8 @@ static void bm_work(struct work_struct *work)
>  			return;
>  		case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM:
>  			// Let's do a bus reset and pick the local node as root, and thus, IRM.
> -			new_root_id = local_id;
> -			goto pick_me;
> +			stand_for_root = true;
> +			break;
>  		case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM:
>  			if (local_id == irm_id) {
>  				// Only acts as IRM.
> @@ -434,60 +435,61 @@ static void bm_work(struct work_struct *work)
>  		case BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM:
>  		default:
>  			card->bm_generation = generation;
> +			stand_for_root = false;
>  			break;
>  		}
>  	}
>  
> -	/*
> -	 * We're bus manager for this generation, so next step is to
> -	 * make sure we have an active cycle master and do gap count
> -	 * optimization.
> -	 */
> -	if (card->gap_count == GAP_COUNT_MISMATCHED) {
> -		/*
> -		 * If self IDs have inconsistent gap counts, do a
> -		 * bus reset ASAP. The config rom read might never
> -		 * complete, so don't wait for it. However, still
> -		 * send a PHY configuration packet prior to the
> -		 * bus reset. The PHY configuration packet might
> -		 * fail, but 1394-2008 8.4.5.2 explicitly permits
> -		 * it in this case, so it should be safe to try.
> -		 */
> -		new_root_id = local_id;
> -		/*
> -		 * We must always send a bus reset if the gap count
> -		 * is inconsistent, so bypass the 5-reset limit.
> -		 */
> -		card->bm_retries = 0;
> -	} else {
> -		// Now investigate root node.
> -		struct fw_device *root_device = fw_node_get_device(root_node);
> -
> -		if (root_device == NULL) {
> -			// Either link_on is false, or we failed to read the
> -			// config rom.  In either case, pick another root.
> -			new_root_id = local_id;
> +	// We're bus manager for this generation, so next step is to make sure we have an active
> +	// cycle master and do gap count optimization.
> +	if (!stand_for_root) {

I realized that the "stand_for_root" local variable would be
ununitialized here at the case of "card->bm_generation == generation".
Let me post take 2.


Regards

Takashi Sakamoto