On 23 August 2016 at 03:43, Shaun Tancheff [off-list ref] wrote:

quoted

quoted

+       if (unmap) {
+               /* If trim is not enabled the cmd is invalid. */
+               if ((dev->horkage & ATA_HORKAGE_NOTRIM) ||
+                   !ata_id_has_trim(dev->id)) {
+                       fp = 1;
+                       bp = 3;
+                       goto invalid_fld;
+               }
+               /* If the request is too large the cmd is invalid */
+               if (n_block > 0xffff * trmax) {
+                       fp = 2;
+                       goto invalid_fld;
+               }

This response should be generally applied to the Write Same (16)
translation, since it is required by SBC,

quoted

+       } else {
+               /* If write same is not available the cmd is invalid */
+               if (!ata_id_sct_write_same(dev->id)) {
+                       fp = 1;
+                       bp = 3;
+                       goto invalid_fld;
+               }

therefore, you should add an n_block check here as well, if you are
going to advertise an Maximum Write Same Length even when the device
supports only SCT Write Same but not TRIM. Most likely you would want
to simply move the existing check one-level up (if the same limit is
advertised no matter TRIM is supported not or not).

Why would we enforce upper level limits on something that doesn't
have any?

If we advertise a limit in our SATL, it makes sense that we should
make sure the behaviour is consistent when we issue a write same
through the block layer / ioctl and when we issue a SCSI Write Same
command directly (e.g. with sg_write_same). IMHO that's pretty much
why SBC would mandate such behaviour as well.

If the upper level, or SG_IO, chooses to set a timeout of 10 hours and
wipe a whole disk it should be free to do so.

That's why I said, "if you are going to advertise an Maximum Write Same Length".