From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) (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 1DD66233931 for ; Fri, 22 May 2026 21:36:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.48 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779485795; cv=none; b=R8V7WGoCGdlG2tctnYsvdcA4N9+6NzDIgrJq96LqWExKMHaHl4F9kkZstqA9CRIkuvTebwISmekms6ltmZ64hBt2qQEeVImhnmfu4b7P6WyCCtoECfmT3g+G3V3JHnAYlGnt24rbIkkHbuQeFH41KExriq8Lb82KmW5ReZ/nE7c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779485795; c=relaxed/simple; bh=aIS7cZHGJDd3LFnie+bBtN7lwbtQRDS9zwaybw0o58g=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=d2I031WrYEM+AgrPzKci9xYUp59mSQOE/fbGM4dwho/GJNLXp8mbt9HVF1ZyApmkK5JpT9zqRy58SBbk+OQkLkiWcS9uBWu+JoJo2OeLDgDTT+/fXnP6L6dAVpinRgys+scCyyiKDorwLna/Wfe/95kDgZsGMvlyVM5Mw1xD48g= 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=XTVc4vUd; arc=none smtp.client-ip=209.85.128.48 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="XTVc4vUd" Received: by mail-wm1-f48.google.com with SMTP id 5b1f17b1804b1-49042aeeb75so18695445e9.1 for ; Fri, 22 May 2026 14:36:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1779485792; x=1780090592; 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=vfZtpNg/xxd2BsQF/1l2YYHH7lYt774iHWExpAvOxcg=; b=XTVc4vUde4GNLTUcS7qivqmTFnWu1GhQVOxSWAWTZb4siKJu89ICaOF0y2EgeXYRXa E7pBhuteMDU6cCsYlb7zsMCZBCBnEuoqjaAfHpDX1DZvPX6Oh0Lodj+TL56QKeR6X7Dn Frb0SJwjAYBnNexDdaMYtxdG6RarY7/78usteD6uNKONhj/h4b/dyGQaNuw9lp2JCI+O ZF7I6nqArM0ShJ4sS+VYofvfJKgtr3HXRJMJNNpFvd+i/1b9g/KbgFSg+i20lt8Sxh73 guUndM0EQ36VDXZH6oAOcCDk9VOMAWgajYUHMKI8Id5eH1bXvLaV/6VHDiyjW4ognpvB 6vkQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779485792; x=1780090592; 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=vfZtpNg/xxd2BsQF/1l2YYHH7lYt774iHWExpAvOxcg=; b=CxvRJBi0Qb49MjRiuKaY8g50BEhqaMQs8nG2NA7vXOdSAfjDo31+27MMkmw/nCMuYX 5klQHhYVfhSvNKgcHE1HYQmOCoz8MiOBuBfGQBHxWQIfDBjOH+idf4AblFVey1SEyuN+ y/+CiIuvditJnDuKw6OSJVjC5uWI9sqgbO/9KH890pA2G3rSsXDyoOHqwYOI+aTW83cb I5SUYTqgkYC0T50fwZFFmui5QgrQF2SZIzz5+dcMDaFnLOGJB1iCA5GAcjCsgQ3gt0bZ ag13wYR5eygwJHVC+qRR/+EqmhOc4yGFuQT67GQWr7dvQy6C7w2d7toABmucd55oxjJK 7lzw== X-Forwarded-Encrypted: i=1; AFNElJ8+CcwOhfp1ACyyoGW6xZ88ElNVdtFJZzVa7ooTlT66XAysfK2I/SQcUiNCMSM/3ceoT4WP19OGG09jF8Q=@vger.kernel.org X-Gm-Message-State: AOJu0YyTL6ZRUtMXduF7vV4ayK0sUl3gBM0FCGGYHX5mWY/ZZJp6HKB/ dQVEHQQQAdtscjrjDWBDmI/RLKIsa5K5AxkVlhc3lhSn6FPSZ3pxUrqF X-Gm-Gg: Acq92OFO59CZgMsQYauEi6e6vU45tGBSPXQAhf4ydjgOlqjX7Zyd+GBz4c08bp7exTW +gTeivR7NAkR/i1PMQkBJ5RUabcNfJf9suUNEZwr8EBNKEYlByBhQMifEhipBwBKWS9AxHhV9sw bfUO49Z9YCPlOLXVKY/nn4wTLr3AxvdymAmMyIt0JjFlph/cjp8LQs8n3uFow+B6cmy+mOD5huR mJXG9j5SDJgjtZ0Fgd78ifqg7EwZ5mAGW/wM4rl0mTLFetkDAfxWHryMNGU5R36/0S+w0AHMcMB DatCks+Tp4m/1F9cPOR9zlPvza0aBjQubm/PaOgDufZ0dIBhVm2DvG7z5eoBARA0sNVWjbBS5G6 bulvSrd25Y0qg1pgr9uwsXTjn9Q3JykNcf3gJGZQVh6QNWxvDL2+/pVDLyDkZjn+wRj774vZk54 bv7rxC9glwdvYLfQzGDDEviNwgvpeLBR6vUUd/WGhE63A7t8+D58wcuLZ88mTnRC8kin6gj6yqH piWSKfRL6rOc85rqyza8KD9vWK1/CsNcgR6XQmp4jpdCLE= X-Received: by 2002:a05:600c:a213:b0:48e:8741:fd42 with SMTP id 5b1f17b1804b1-490424ada3bmr53332355e9.12.1779485792276; Fri, 22 May 2026 14:36:32 -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 5b1f17b1804b1-4904527dbf3sm63950535e9.6.2026.05.22.14.36.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 22 May 2026 14:36:31 -0700 (PDT) Message-ID: <4a1b13f4-9c68-4f4c-a676-fd61e2aeeab0@gmail.com> Date: Fri, 22 May 2026 23:36:30 +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 1/2] net: sfp: apply I2C adapter quirks to limit block size 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-2-jelonek.jonas@gmail.com> <20260520234204.565333-1-kuba@kernel.org> From: Jonas Jelonek In-Reply-To: <20260520234204.565333-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: apply I2C adapter quirks to limit block size > > This patch modifies the SFP driver to respect I2C adapter quirks when > configuring the I2C block size. It restricts the maximum block size > based on the adapter's max_read_len and max_write_len fields. > >> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c >> index bd970f753beb6..e58e29a1e8d27 100644 >> --- a/drivers/net/phy/sfp.c >> +++ b/drivers/net/phy/sfp.c >> @@ -807,21 +807,29 @@ static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr, >> >> static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c) >> { > [ ... ] > >> } else { >> sfp->i2c = NULL; >> return -EINVAL; >> } >> >> + if (i2c->quirks && i2c->quirks->max_read_len) >> + max_block_size = min(max_block_size, i2c->quirks->max_read_len); >> + if (i2c->quirks && i2c->quirks->max_write_len) >> + max_block_size = min(max_block_size, i2c->quirks->max_write_len); > Does this limit the read block size incorrectly based on the write limit? > > The write portion of an I2C read transfer is always 1 byte for the EEPROM > offset, regardless of the read block size. > > If an adapter has a max_write_len of 1, max_block_size will drop to 1. > Since sfp_hwmon_probe() checks if sfp->i2c_max_block_size < 2 to ensure > atomic 16-bit reads, does this inadvertently disable hardware monitoring for > adapters that have a small write limit but sufficient read capabilities? > > Also, for actual write transfers, sfp_i2c_write() prepends a 1-byte register > address to the payload. If max_block_size is set exactly to max_write_len, > won't the resulting message length be 1 + max_write_len, which still exceeds > the adapter's quirk limit? > > Additionally, looking at sfp_i2c_write(), it does not appear to loop or > chunk the buffer using sfp->i2c_max_block_size. Does this mean the new > limit will be ignored during writes, and the function will still attempt > to transfer the entire buffer in a single operation? Valid concern but it seems like this doesn't have a practical impact right now. Having a closer look at in-tree drivers shows that (a) i2c-tegra (read-cap: 4096 | write-cap: 4084) (b) i2c-tegra194 (read-cap: no | write-cap: 65524) (c) i2c-mlxbf (read-cap: 128 | write-cap: 127) (d) i2c-qcom-cci (read-cap: 12 | write-cap: 10/11) seem to have an asymmetry between read cap and write cap. However, the actual limits for (1), (2) and (3) are far beyond the 16 bytes the SFP driver uses. For (d) it is below but would trigger the hwmon disable. Moreover, qcom-cci seems to be a driver running camera sensors, nothing that will ever be used for SFP cages. Thus I would acknowledge this but not add a fix for that. On other opinions, please raise your hand. Then I might provide another patch implementing per-direction caps, or any other suggested solution. >> + >> + sfp->i2c_max_block_size = max_block_size; >> return 0; >> } Regards, Jonas