> On 30. Jan 2025, at 20.27, John Meneghini <jmeneghi@xxxxxxxxxx> wrote:
>
> One suggestion here.
>
> On 1/20/25 2:49 PM, Kai Mäkisara wrote:
>> If the value read from the file is 1, reads and writes from/to the
...
>> --- a/Documentation/scsi/st.rst
>> +++ b/Documentation/scsi/st.rst
>> @@ -157,6 +157,11 @@ enabled driver and mode options. The value in the file is a bit mask where the
>> bit definitions are the same as those used with MTSETDRVBUFFER in setting the
>> options.
>> +Each directory contains the entry 'reset_blocked'. If this value is one,
>
> I suggest calling this 'position_lost' or 'position_unknown' rather than 'reset_blocked'.
>
>> +reading and writing to the device is blocked after device reset. Most
>> +/**
...
>> + * reset_blocked_show - Value 1 indicates that reads, writes, etc. are blocked
>
> position_lost_show
Yes. The function name should be changed when the file name is changed.
>
>> + * because a device reset has occurred and no operation positioning the tape
>> + * has been issued.
>> + * @dev: struct device
>> + * @attr: attribute structure
>> + * @buf: buffer to return formatted data in
>> + */
>> +static ssize_t reset_blocked_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct st_modedef *STm = dev_get_drvdata(dev);
>> + struct scsi_tape *STp = STm->tape;
>> +
>> + return sprintf(buf, "%d", STp->pos_unknown);
> ^^^^^^^^^^^^^^
>
> I'd like the name of this function/sysfs variable to more closely match the code
> that drives this state.
I am not too fond of the name "reset_blocked", but I had to choose something :-)
This name is based on the phenomenon causing the situation (device reset) and
what it causes (blocks many st operations).
I think "reset" should be part of the name. Device reset may mean that the tape
position is changed (rewind), but it can also mean more: the device settings are
reset to the saved values. Patch 1/4 restores the values set using st, but it does
not restore the values set by other means (e.g., encryption). The user should
remember to re-set also these before continuing.
Combining the above and your suggestion, what about "position_lost_in_reset"
or "pos_lost_in_reset"? (Whatever the name is, the user should check what
has really happened. The name should point to the correct direction,
but it should be short enough.)
Thanks!
