Re: PMBus memory overflow

[Matt Corallo <yalbrymrb@mattcorallo.com>]

On 4/19/25 3:05 PM, Guenter Roeck wrote:
> On 4/19/25 10:53, Matt Corallo wrote:
>>
>> Sure, of course. Obviously easier to do in pmbus_peek if you have any suggested logs you want. The 
>> device caps output is (and still never hit my BAD LEN printf):
>>
>> ./a.out -b /dev/i2c-3 -l -v 0x58
>> PMBus slave on /dev/i2c-3, address 0x58
>>
>> Inventory Data:
>>    Manufacturer:        FSP-GROUP
>>    Model:        FSP520-20RAB
>>    Revision:        (null)
>>    Built on:
>>    Serial:
>>    IC Device:        PIC24FJ32GA004
>>
>> PMBus revisions (0x22):    part I, ver 1.1; part II, ver 1.2
>> Capabilities (0x90):    PEC, SMBALERT#, 100 KHz
>>
>> Supported Commands:
>>    00 page                      rw u8 (bitmask)
>>    01 operation                 rw u8 (bitmask)
>>    02 on_off_config             rw u8 (bitmask)
>>    03 clear_fault                w nodata
>>    05 page_plus_read             w block
>>    19 capability                r  u8 (bitmask)
>>    1a query                     rw process_call
> 
> pmbus_peek uses the query command to determine if a command
> is supported or not. It doesn't try to read other commands.
> 
>>    1b smbalert_mask             rw block
>>    20 vout_mode                 r  u8 (bitmask)
>>    30 coefficients              r  process_call
>>    3b fan_command_1             rw s16 (LINEAR)
>>    79 status_word               r  u16 (bitmask)
>>    7a status_vout               r  u8 (bitmask)
>>    7b status_iout               r  u8 (bitmask)
>>    7c status_input              r  u8 (bitmask)
>>    7d status_temperature        r  u8 (bitmask)
>>    81 status_fans_1_2           rw u8 (bitmask)
>>    86 read_ein                  r  block(6), Energy counter (DIRECT)
>>    87 read_eout                 r  block(6), Energy counter (DIRECT)
>>    88 read_vin                  r  s16 (LINEAR), Volts
>>    89 read_iin                  r  s16 (LINEAR), Amperes
>>    8b read_vout                 r  x16 (VOUT_MODE), Volts
>>    8c read_iout                 r  s16 (LINEAR), Amperes
>>    8d read_temperature_1        r  s16 (LINEAR), degrees Celsius
>>    8e read_temperature_2        r  s16 (LINEAR), degrees Celsius
>>    90 read_fan_speed_1          r  s16 (LINEAR)
>>    95 read_frequency            r  s16 (LINEAR)
>>    96 read_pout                 r  s16 (LINEAR), Watts
>>    97 read_pin                  r  s16 (LINEAR), Watts
>>    98 pmbus_revision            r  u8 (bitmask)
>>    99 mfr_id                    r  block, ISO 8859/1 string
>>    9a mfr_model                 r  block, ISO 8859/1 string
>>    9d mfr_date                  r  block, ISO 8859/1 string
>>    9e mfr_serial                r  block, ISO 8859/1 string
> 
> That also means it does not try to read mfr_revision or mfr_location
> since that is not supported. The PMBus driver in the kernel does
> try to read those (it doesn't use the query command to determine
> if a command is supported or not).
> 
> My suspicion is that the power supply returns something (but not
> valid SMBus block data) when reading those commands. If modifying
> the kernel to find out is not easy, another option might be to
> enable smbus tracing. Would it be possible to do that ?
> Another option might be to modify the pmbus_peek command to read
> those registers, or maybe even better to use i2cget to read the
> data directly.

Hmm, doesn't seem to trigger it at least with pmbus_peek.c, the following diff still doesn't hit the 
too big prints:

diff -U5 orig.c pmbus_peek.c
--- orig.c	2025-04-19 19:28:44.314887951 +0000
+++ pmbus_peek.c	2025-04-19 19:26:42.721194496 +0000
@@ -619,16 +619,19 @@
  	}

  	if (data.block[0] <= read_len) {
  		if (data.block[0] > 32) {
  			/* NOTE:  this probably won't be visible */
+fprintf(stderr, "\nERR too big 32!\n");
  			retval = -EFBIG;
  			goto try_i2c;
  		}
  		retval = read_len = data.block[0];
-	} else
+	} else {
+fprintf(stderr, "\nERR too big!\n");
  		retval = -E2BIG;
+}
  	memcpy(read_buf, &data.block[1], read_len);

  	return retval;

  try_i2c:
@@ -1016,11 +1019,11 @@
  	 *
  	 * NOTE:  this assumes we can check prefixes for PMB_MFR_EXT(x)
  	 * and PMB_EXT(x) operations, even though we can't query those
  	 * operations directly...
  	 */
-	if (is_pmb_extended(cmd))
+	//if (is_pmb_extended(cmd))
  		return -1;

  	if (pmdev->op[PMB_QUERY] == &unsupported || pmdev->no_query)
  		return -1;

@@ -1044,12 +1047,12 @@
  	int status;

  	/* For non-queryable devices we'll still try to read inventory
  	 * data strings.  That should be harmless but informative.
  	 */
-	if (checksupport(pmdev, cmd) == 0)
-		return NULL;
+	//if (checksupport(pmdev, cmd) == 0)
+	//	return NULL;

  	memset(buf, 0, sizeof buf);
  	status = pmbus_read_block(pmdev, cmd, sizeof buf - 1, buf);
  	return (status > 0) ? strdup((void *)buf) : NULL;
  }

> Somewhat unrelated to this, it might be time to revisit using the
> QUERY command to determine what is supported. If only I had an endless
> amount of time ... oh well. When I wrote the kernel driver, none of
> the chips I had available at the time supported that command :-(.
> 
> Thanks,
> Guenter
> 
>