Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NetBSD compatibility: Increase daynaport minimum packet size to 128 bytes (#1334) #1335

Merged
merged 3 commits into from
Nov 14, 2023
Merged

NetBSD compatibility: Increase daynaport minimum packet size to 128 bytes (#1334) #1335

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7b6f10
NetBSD compatibility: Increase daynaport minimum packet size to 128 b…
uweseimet Nov 10, 2023
052899c
Update comment
uweseimet Nov 10, 2023
72f2fed
Update inconsistent comment
uweseimet Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions cpp/devices/scsi_daynaport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// Copyright (C) 2020 akuker
// Copyright (C) 2014-2020 GIMONS
// Copyright (C) 2001-2006 ＰＩ．([email protected])
// Copyright (C) 2023 Uwe Seimet
//
// Licensed under the BSD 3-Clause License.
// See LICENSE file in the project root folder.
Expand Down Expand Up @@ -212,13 +213,14 @@ int SCSIDaynaPort::Read(cdb_t cdb, vector<uint8_t>& buf, uint64_t)
// response->flags = e_no_more_data;
// }
int size = rx_packet_size;
if (size < 64) {
// A frame must have at least 64 bytes (see https://github.com/PiSCSI/piscsi/issues/619)
if (size < 128) {
// A frame must have at least 128 bytes (see https://github.com/PiSCSI/piscsi/issues/619
// and https://github.com/PiSCSI/piscsi/issues/1098.
// Note that this work-around breaks the checksum. As currently there are no known drivers
// that care for the checksum, and the Daynaport driver for the Atari expects frames of
// 64 bytes it was decided to accept the broken checksum. If a driver should pop up that
// breaks because of this, the work-around has to be re-evaluated.
size = 64;
// that care for the checksum, the Daynaport driver for the Atari expects frames of 64 bytes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I missed the whole discussion elsewhere, but you're saying that the Atari driver expects 64 bit frames but can handle 128 bit frames too because it's a multiple of 64?

(The resulting code comment reads contradictory to me)

Also, I assume @benjamink is helping you test this change with the Mac drivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this change successfully with the Atari driver, i.e. it can cope with 128 bytes. I guess anything > 64 bytes is fine, but 128 bytes definitely work.
Yes, @benjamink is doing the Mac tests.
The comment is indeed inconsistent. I just updated it, hope it is clearer now.

// and the driver for NetBSD 128 bytes it was decided to accept the broken checksum.
// If a driver should pop up that breaks because of this, the work-around has to be re-evaluated.
size = 128;
}
SetInt16(buf, 0, size);
SetInt32(buf, 2, tap.HasPendingPackets() ? 0x10 : 0x00);
Expand Down
1 change: 1 addition & 0 deletions cpp/devices/scsi_daynaport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// Copyright (C) 2020 akuker
// Copyright (C) 2014-2020 GIMONS
// Copyright (C) 2001-2006 ＰＩ．([email protected])
// Copyright (C) 2023 Uwe Seimet
//
// Licensed under the BSD 3-Clause License.
// See LICENSE file in the project root folder.
Expand Down