From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B29172475C8 for ; Sat, 14 Jun 2025 10:56:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1749898563; cv=none; b=X5JvJIrtSAYgx1URgKSq3RkPGgzDzWMB0g+D9xnMVFN+sd24sW5uzuDwMjjngaJO9l+AKB2eTgeXq/f/+XKjld6M3WwZxbYo8TfeaqJypQjLNAofjFVvyDfGB6+0td/74Si6Mw5ZHnCb0zNmUEsYv7GAc1HmQ18eujn5mElnvcQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1749898563; c=relaxed/simple; bh=v9JAZMbAlGj+PI2KBTAH+4c6GQ22Vu0QZw0EvB4pGcs=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=KBHfqODCKqWWNtkgaJ0KK3rcn3ssc4VlOVgF+QMIt5JfME2cm5vVtWv81E1u2LJBFTCsBn0tEqRoFFmOxO7eXb19J0Y5+MQfEujqCqlXViGPbB0gP5bZ624cT+mrwTynZfKpQLBDn1Yg6BRRtFw0Ailx902PgNJQRpdGgYdZg+8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=iFUpQF3P; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="iFUpQF3P" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1749898560; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=q3cUmQ0MyZkdcrlxR7yc+f3znrveTY8lEdfw/tNRmdE=; b=iFUpQF3PGWvS3829tQDHbqT6k6NRH0xIpM51aI59ns5QXiRvQWjN1HHCxdMmQJxBUL26/C Td5+pWkiL/+f6/M4gzKEzY8XHl6EspXyt/FQBRgWFXlyX8WyqOnbX6Y3jJcoEIuiVlHAhl X9cQ24HtciBd3KcpsKN4MN+1trtxTe4= Received: from mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-617-ypRNLI9eP0mJlHcLtYRiCA-1; Sat, 14 Jun 2025 06:55:57 -0400 X-MC-Unique: ypRNLI9eP0mJlHcLtYRiCA-1 X-Mimecast-MFC-AGG-ID: ypRNLI9eP0mJlHcLtYRiCA_1749898555 Received: from mx-prod-int-02.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-02.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.15]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 21A86195608B; Sat, 14 Jun 2025 10:55:54 +0000 (UTC) Received: from [10.45.224.53] (unknown [10.45.224.53]) by mx-prod-int-02.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 247F81956094; Sat, 14 Jun 2025 10:55:45 +0000 (UTC) Message-ID: Date: Sat, 14 Jun 2025 12:55:44 +0200 Precedence: bulk X-Mailing-List: linux-doc@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next v9 06/14] dpll: zl3073x: Fetch invariants during probe To: Vadim Fedorenko , netdev@vger.kernel.org Cc: Arkadiusz Kubalewski , Jiri Pirko , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Prathosh Satish , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Jonathan Corbet , Jason Gunthorpe , Shannon Nelson , Dave Jiang , Jonathan Cameron , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, Michal Schmidt , Petr Oros References: <20250612200145.774195-1-ivecera@redhat.com> <20250612200145.774195-7-ivecera@redhat.com> Content-Language: en-US From: Ivan Vecera In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.0 on 10.30.177.15 On 13. 06. 25 9:13 odp., Vadim Fedorenko wrote: >> +    synth->enabled = FIELD_GET(ZL_SYNTH_CTRL_EN, synth_ctrl); >> +    synth->dpll = FIELD_GET(ZL_SYNTH_CTRL_DPLL_SEL, synth_ctrl); >> + >> +    dev_dbg(zldev->dev, "SYNTH%u is %s and driven by DPLL%u\n", index, >> +        synth->enabled ? "enabled" : "disabled", synth->dpll); >> + >> +    guard(mutex)(&zldev->multiop_lock); > > Not a strong suggestion, but it would be good to follow netdev style > (same for some previous functions): Hi Vadim, I'm using guard() on places (functions) where it is necessary to hold the lock from that place to the end of the function. Due to this scoped_guard() does not give any advantage. Using classic mutex_lock() and mutex_unlock() would only increases the risks of locking-related bugs. Also manual locking enforces to use mutex_unlock() or goto in all error paths after taking lock. > https://docs.kernel.org/process/maintainer-netdev.html#using-device- > managed-and-cleanup-h-constructs > > "Use of guard() is discouraged within any function longer than 20 lines, > scoped_guard() is considered more readable. Using normal lock/unlock is > still (weakly) preferred." > >> + >> +    /* Read synth configuration */ >> +    rc = zl3073x_mb_op(zldev, ZL_REG_SYNTH_MB_SEM, ZL_SYNTH_MB_SEM_RD, >> +               ZL_REG_SYNTH_MB_MASK, BIT(index)); >> +    if (rc) >> +        return rc; >> + >> +    /* The output frequency is determined by the following formula: >> +     * base * multiplier * numerator / denominator >> +     * >> +     * Read registers with these values >> +     */ >> +    rc = zl3073x_read_u16(zldev, ZL_REG_SYNTH_FREQ_BASE, &base); >> +    if (rc) >> +        return rc; >> + >> +    rc = zl3073x_read_u32(zldev, ZL_REG_SYNTH_FREQ_MULT, &mult); >> +    if (rc) >> +        return rc; >> + >> +    rc = zl3073x_read_u16(zldev, ZL_REG_SYNTH_FREQ_M, &m); >> +    if (rc) >> +        return rc; >> + >> +    rc = zl3073x_read_u16(zldev, ZL_REG_SYNTH_FREQ_N, &n); >> +    if (rc) >> +        return rc; >> + ---> You have to keep the lock to here. >> +    /* Check denominator for zero to avoid div by 0 */ >> +    if (!n) { >> +        dev_err(zldev->dev, >> +            "Zero divisor for SYNTH%u retrieved from device\n", >> +            index); >> +        return -EINVAL; >> +    } >> + >> +    /* Compute and store synth frequency */ >> +    zldev->synth[index].freq = div_u64(mul_u32_u32(base * m, mult), n); >> + >> +    dev_dbg(zldev->dev, "SYNTH%u frequency: %u Hz\n", index, >> +        zldev->synth[index].freq); >> + >> +    return rc; >> +} This kind of function (above) is mailbox-read: 1. Take lock 2. Ask firmware to fill mailbox latch registers 3. Read latch1 4. ... 5. Unlock But in later commits there are mailbox-write functions that: 1. Take lock 2. Ask firmware to fill mailbox latch registers 3. Write or read-update-write latch registers 4. ... 5. Ask firmware to update HW from the latch registers (commit) 6. Unlock Step 5 here is usually represented by: return zl3073x_mb_op(zldev, ZL_REG_*_MB_SEM, ZL_*_MB_SEM_RD, ZL_REG_*_MB_MASK, BIT(index)); and here is an advantage of guard() that unlocks the mutex automatically after zl3073x_mb_op() and prior returning its return value. Thanks, Ivan