From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 39E8E38888B for ; Fri, 22 May 2026 21:24:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.45 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779485064; cv=none; b=P+PhpmCz2DwlPY9kFQYp7ZhcnXatumq96VlvWUnEd5ZEedH9lbesVeSNrL5loC4rqtYqaPvBAr9QQeHhxSdfeutjreuDHvLSs+kRF7RjEimFOp1wa0GU5f3ej7yj8Fx3DhrtZzCTvbpQP+T5ZE/ECyeNm327N4CwU+QWcTAGMjw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779485064; c=relaxed/simple; bh=4X6wzxH9CRi7D5/xqnYw5mUklSE4bIzW4b0U7UOUrWU=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=VaWF5QO72FCD63RUY99fBb1LnascIHt2lA1P6F+tjEqjtIiFN/PCl/A0d/nL/tq/9iO3y9r3zdMT2abqe1kDC0OiD/B0o387zIc+f/01jJVMU/HBwsCwMVNEm7RBulqxA4dQvjPnHEXHjBF7LZOeO3GZvle/vhSXqdLeTKeTvCM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=Mvr2ojTI; arc=none smtp.client-ip=209.85.128.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Mvr2ojTI" Received: by mail-wm1-f45.google.com with SMTP id 5b1f17b1804b1-49048e043e5so5889505e9.1 for ; Fri, 22 May 2026 14:24:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1779485062; x=1780089862; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=H1dYQAJtuPMBNLTKwJZ/6YxJZAmjypXguzETEzIyllw=; b=Mvr2ojTIBVe2o3ED+VIKXcZQr0Oy2EYAJBgUjy5/t+zQWSGShwYEHO2jg8NGot9yPk tZ0rYKSA/kn9FdjqoDmMYPsPGZKGTMn6IvMWWhB8Lnchpwmgz8MrhG69zcbmpe4RCuBG 71nq0YdRF8nZ68xT1H+s2DfhSEUIospWShlrl/FYDqHkRK4Q4MVRpsZBbyETvjef3VyS JvBkNgLxEiOnvBOZ3Ni7zszj6HtUcqTzYb7bBJlZAzyVZcacYBghvV2WLWjMgBoCtUr0 IoCbR0hrWIsfHv7fpJ7GQ1f7wew6zRtdYyfysDZLjw0xb5fMzhaxJlAATcI5/AievCs5 XuxA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779485062; x=1780089862; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=H1dYQAJtuPMBNLTKwJZ/6YxJZAmjypXguzETEzIyllw=; b=Jn2jxEUA6cPddGDtIG+ytEcn+lAEWgP0c/mpERrMz3FIFYptmCuuk31Qgix9j8/Bn3 6zfTP7okuo8KDHw0RTYkhri6gH2USRjO1vYAigFUduPjgidD3bxwJBgu6wBC5LXdPpHo Ke7gNXQQjvJRkb1LtdqNUbznkQ8YX/AXkyos+ZjXKBtYHjaOg3RKAFClr6HCu7IKrl+q qM5BO5fYTQ06MOTIa7kO0cLLB13O1RxZEA5RGjr5/0OA+POg71o1CKPS6kY6WOu90uWh gu3NlDns7gMNasGPOZkC2lEoglKhPR2TtTZyU1ThWvGf5vPbhCrrrgTjihD8g8dQCaZu a/xA== X-Forwarded-Encrypted: i=1; AFNElJ8bxXDSyjW36ivOBotbWZYntugZlHDQwgW3Q7dBT21bmKVOdtCkMx+PGT89qhAjpTcZqnS21OWOlA8b/sQ=@vger.kernel.org X-Gm-Message-State: AOJu0Yzgme9o7qVE10swr98jcNjO6QwEI/d2o5fa++Xw/HAihhY/s2jc GEYFjfe9MdoMF8hlFlfwLC56UW50otOCyDiY4PF+O52QalBm4LpIz4qy X-Gm-Gg: Acq92OEMKgSIUSSg1gdc1LY/4flnGg5WVZvvfYgvWaaFCDxUUmAKF6DhJ1/7/goSqTR KeW/pzVF2fllG8TwFEaBR/v72EokxG7YOsCUDbQxzGM1nTRBlfMa0T4jopvqCf7RO7Eg+uITivg eOJS7KJ/t3HLMwNf54VZEKk6bNtx72YYm8hnUPd2Zxgr0jBrfzcG9clqH4iSOTeecb14YEgNSVE +y2kQub/fe6pxz53hBsBdcVK75muwbLagIxsPtdjsYT+x5uWhmFyoJPRjkXRdOcUfdmdWE7ZMQi 5SG9CEXmw8QF4+P3gLh4BRuNM0k/w5ClWTYWQwcjVX0Gx3EbPKGGXVJYsBrs7sktGNPBvWRBTlY m9G0q9cT2/hQWfs7kXXgNk5NNV+l+w0avUXm0jEoAp6AIrkzvc4AXsbIUy+jfOKu05/rDZk5tGy f+T9DiLmtxPDQTCf4xaSr7MJ3srVJwWCW8L7b1XkK9/MPvfqxP92BrSzFeb8qBj3msEl1PsePeA fVqeZuDpVi9s2aOmAbG1jqSwVWvJn/SXCK8y34kMZ6s+Vo= X-Received: by 2002:a05:600c:6217:b0:490:44d0:11b6 with SMTP id 5b1f17b1804b1-49044d01257mr74805245e9.12.1779485061409; Fri, 22 May 2026 14:24:21 -0700 (PDT) Received: from ?IPV6:2001:9e8:f11f:2b01:59e8:aae0:c8be:3081? ([2001:9e8:f11f:2b01:59e8:aae0:c8be:3081]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-45eb6ccd211sm8562544f8f.10.2026.05.22.14.24.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 22 May 2026 14:24:20 -0700 (PDT) Message-ID: Date: Fri, 22 May 2026 23:24:20 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next v8 2/2] net: sfp: extend SMBus support Content-Language: en-US To: Jakub Kicinski Cc: linux@armlinux.org.uk, andrew@lunn.ch, hkallweit1@gmail.com, davem@davemloft.net, edumazet@google.com, pabeni@redhat.com, maxime.chevallier@bootlin.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, bjorn@mork.no, horms@kernel.org References: <20260516135442.2234729-3-jelonek.jonas@gmail.com> <20260520234208.565366-1-kuba@kernel.org> From: Jonas Jelonek In-Reply-To: <20260520234208.565366-1-kuba@kernel.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi, On 21.05.26 01:42, Jakub Kicinski wrote: > This is an AI-generated review of your patch. The human sending this > email has considered the AI review valid, or at least plausible. > --- > net: sfp: extend SMBus support > > Extend SFP SMBus read/write helpers to use SMBus word access and I2C block > access when advertised by the adapter, picking the largest supported transfer > per chunk. > >> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c >> --- a/drivers/net/phy/sfp.c >> +++ b/drivers/net/phy/sfp.c >> @@ -756,50 +757,110 @@ static int sfp_i2c_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf, >> [ ... ] >> -static int sfp_smbus_byte_read(struct sfp *sfp, bool a2, u8 dev_addr, >> - void *buf, size_t len) >> +static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf, >> + size_t len) >> { >> - union i2c_smbus_data smbus_data; >> + union i2c_smbus_data smbus_data = {0}; >> u8 bus_addr = a2 ? 0x51 : 0x50; >> + size_t this_len, transferred; >> + u32 functionality; >> u8 *data = buf; >> int ret; >> >> - while (len) { >> - ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0, >> - I2C_SMBUS_READ, dev_addr, >> - I2C_SMBUS_BYTE_DATA, &smbus_data); >> - if (ret < 0) >> - return ret; >> + functionality = i2c_get_functionality(sfp->i2c); >> >> - *data = smbus_data.byte; >> + while (len) { >> + this_len = min(len, sfp->i2c_block_size); >> + > Does this introduce an -EINVAL regression for ethtool -m during the initial > 300ms module insertion window? > > When an SFP module is inserted, sfp_check_state() sets the SFP_F_PRESENT > flag and schedules sfp_sm_mod_probe() to run after a 300ms delay. > Because sfp->i2c_block_size isn't initialized until sfp_sm_mod_probe() > runs, it remains 0 during this window. > > If ethtool -m is run during this 300ms window, sfp_module_eeprom() proceeds > because SFP_F_PRESENT is set. This means this_len will evaluate to 0. > For adapters supporting I2C_FUNC_SMBUS_READ_I2C_BLOCK, the code attempts > a 0-length I2C block read, which the I2C subsystem core explicitly rejects > with -EINVAL. > > Prior to this patch, sfp_smbus_byte_read() bypassed the block size entirely > and safely executed 1-byte transfers during the 300ms window. > > This also seems to expose a pre-existing deadlock in the pure I2C path. > For pure I2C controllers, sfp_i2c_read() is used. If this_len evaluates > to 0, i2c_transfer() is called with a 0-length read message. If the I2C > adapter returns success, the loop variables remain unchanged, causing an > infinite loop while holding rtnl_lock, which deadlocks the networking stack. > > Is there a way to prevent reads before sfp->i2c_block_size is properly > initialized, or fall back to 1-byte reads when the block size is 0? Valid finding. I would fix this by setting sfp->i2c_block_size in sfp_i2c_configure to the known adapter maximum size at that point. This way, it is filled non-zero and the issue cannot come up. If there's another suggestion, please raise your hand. >> + if (functionality & I2C_FUNC_SMBUS_READ_I2C_BLOCK) { >> + smbus_data.block[0] = this_len; >> + ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0, >> + I2C_SMBUS_READ, dev_addr, >> + I2C_SMBUS_I2C_BLOCK_DATA, &smbus_data); >> + if (ret < 0) >> + return ret; >> + >> + memcpy(data, &smbus_data.block[1], this_len); >> + transferred = this_len; Regards, Jonas