It was <2020-09-07 pon 20:18>, when Andrew Lunn wrote:
>> > On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote:
>> >> +++ b/drivers/net/ethernet/asix/ax88796c_ioctl.c
>> >
>> > This is an odd filename. The ioctl code is wrong anyway, but there is
>> > a lot more than ioctl in here. I suggest you give it a new name.
>> >
>>
>> Sure, any suggestions?
>
> Sorry, i have forgotten what is actually contained.
IOCTL handler (.ndo_do_ioctl), ethtool ops, and a bunch of hw control
functions.
> Does it even need to be a separate file?
It doesn't need, but I think it makes sense to keep ioctl and ethtool
stuff in a separate file. Some of the hw control function look like they
might change after using phylib.
>> >> +u8 ax88796c_check_power(struct ax88796c_device *ax_local)
>> >
>> > bool ?
>>
>> OK.
>>
>> It appears, however, that 0 means OK and 1 !OK. Do you think changing to
>> TRUE and FALSE (or FALSE and TRUE) is required?
>
> Or change the name, ax88796c_check_power_off()? I don't really care,
> so long as it is logical and not surprising.
>
Good idea, thanks.
>> >> + AX_READ_STATUS(&ax_local->ax_spi, &ax_status);
>> >> + if (!(ax_status.status & AX_STATUS_READY)) {
>> >> +
>> >> + /* AX88796C in power saving mode */
>> >> + AX_WAKEUP(&ax_local->ax_spi);
>> >> +
>> >> + /* Check status */
>> >> + start_time = jiffies;
>> >> + do {
>> >> + if (time_after(jiffies, start_time + HZ/2)) {
>> >> + netdev_err(ax_local->ndev,
>> >> + "timeout waiting for wakeup"
>> >> + " from power saving\n");
>> >> + break;
>> >> + }
>> >> +
>> >> + AX_READ_STATUS(&ax_local->ax_spi, &ax_status);
>> >> +
>> >> + } while (!(ax_status.status & AX_STATUS_READY));
>> >
>> > include/linux/iopoll.h
>> >
>>
>> Done. The result seems only slightly more elegant since the generic
>> read_poll_timeout() needs to be employed.
>
> Often code like this has bugs in it, not correctly handling the
> scheduler sleeping longer than expected. That is why i point people at
> iopoll, no bugs, not elegance.
>
>> The manufacturer says
>>
>> The AX88796C integrates on-chip Fast Ethernet MAC and PHY, […]
>>
>> There is a single integrated PHY in this chip and no possiblity to
>> connect external one. Do you think it makes sense in such case to
>> introduce the additional layer of abstraction?
>
> Yes it does, because it then uses all the standard phylib code to
> drive the PHY which many people understand, is well tested, etc. It
> will make the MAC driver smaller and probably less buggy.
>
Good point. I need to figure out how to do it. Can you point (from the
top fou your head) a driver which does it for a simmilarly integrated
device?
>> >> +static char *macaddr;
>> >> +module_param(macaddr, charp, 0);
>> >> +MODULE_PARM_DESC(macaddr, "MAC address");
>> >
>> > No Module parameters. You can get the MAC address from DT.
>>
>> What about systems without DT? Not every bootloader is sophisicated
>> enough to edit DT before starting kernel. AX88786C is a chip that can be
>> used in a variety of systems and I'd like to avoid too strong
>> assumptions.
>
> There is also a standardised way to read it from ACPI. And you can set
> it using ip link set. DaveM will likely NACK a module parameter.
>
I am not arguing to keep the parameter at any cost, but I would really
like to know if there is a viable alternative for DT and ACPI. This chip
is for smaller systems which not necessarily implement advanced
bootloaders (and DT).
>> >> +MODULE_AUTHOR("ASIX");
>> >
>> > Do you expect ASIX to support this?
>>
>> No.
>>
>> > You probably want to put your name here.
>>
>> I don't want to be considered as the only author and as far as I can
>> tell being mentioned as an author does not imply being a
>> maintainer. Do you think two MODULE_AUTHOR()s be OK?
>
> Can you have two? One with two names listed is O.K.
>
According to module.h
/*
* Author(s), use "Name <email>" or just "Name", for multiple
* authors use multiple MODULE_AUTHOR() statements/lines.
*/
>> >> +
>> >> + phy_status = AX_READ(&ax_local->ax_spi, P0_PSCR);
>> >> + if (phy_status & PSCR_PHYLINK) {
>> >> +
>> >> + ax_local->w_state = ax_nop;
>> >> + time_to_chk = 0;
>> >> +
>> >> + } else if (!(phy_status & PSCR_PHYCOFF)) {
>> >> + /* The ethernet cable has been plugged */
>> >> + if (ax_local->w_state == chk_cable) {
>> >> + if (netif_msg_timer(ax_local))
>> >> + netdev_info(ndev, "Cable connected\n");
>> >> +
>> >> + ax_local->w_state = chk_link;
>> >> + ax_local->w_ticks = 0;
>> >> + } else {
>> >> + if (netif_msg_timer(ax_local))
>> >> + netdev_info(ndev, "Check media status\n");
>> >> +
>> >> + if (++ax_local->w_ticks == AX88796C_WATCHDOG_RESTART) {
>> >> + if (netif_msg_timer(ax_local))
>> >> + netdev_info(ndev, "Restart autoneg\n");
>> >> + ax88796c_mdio_write(ndev,
>> >> + ax_local->mii.phy_id, MII_BMCR,
>> >> + (BMCR_SPEED100 | BMCR_ANENABLE |
>> >> + BMCR_ANRESTART));
>> >> +
>> >> + if (netif_msg_hw(ax_local))
>> >> + ax88796c_dump_phy_regs(ax_local);
>> >> + ax_local->w_ticks = 0;
>> >> + }
>> >> + }
>> >> + } else {
>> >> + if (netif_msg_timer(ax_local))
>> >> + netdev_info(ndev, "Check cable status\n");
>> >> +
>> >> + ax_local->w_state = chk_cable;
>> >> + }
>> >> +
>> >> + ax88796c_set_power_saving(ax_local, ax_local->ps_level);
>> >> +
>> >> + if (time_to_chk)
>> >> + mod_timer(&ax_local->watchdog, jiffies + time_to_chk);
>> >> +}
>> >
>> > This is not the normal use of a watchdog in network drivers. The
>> > normal case is the network stack as asked the driver to do something,
>> > normally a TX, and the driver has not reported the action has
>> > completed. The state of the cable should not make any
>> > difference. This does not actually appear to do anything useful, like
>> > kick the hardware to bring it back to life.
>> >
>>
>> Maybe it's the naming that is a problem. Yes, it is not a watchdog, but
>> rather a periodic housekeeping and it kicks hw if it can't negotiate
>> the connection. The question is: should the settings be reset in such case.
>
> Let see what is left once you convert to phylib.
>
OK.
>> >> + struct net_device *ndev = ax_local->ndev;
>> >> + int status;
>> >> +
>> >> + do {
>> >> + if (!(ax_local->checksum & AX_RX_CHECKSUM))
>> >> + break;
>> >> +
>> >> + /* checksum error bit is set */
>> >> + if ((rxhdr->flags & RX_HDR3_L3_ERR) ||
>> >> + (rxhdr->flags & RX_HDR3_L4_ERR))
>> >> + break;
>> >> +
>> >> + if ((rxhdr->flags & RX_HDR3_L4_TYPE_TCP) ||
>> >> + (rxhdr->flags & RX_HDR3_L4_TYPE_UDP)) {
>> >> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>> >> + }
>> >> + } while (0);
>> >
>> >
>> > ??
>> >
>>
>> if() break; Should I use goto?
>
> Sorry, i was too ambiguous. Why:
>
> do {
> } while (0);
>
> It is an odd construct.
As to "why" — you have correctly spotted, this is a vendor driver I am
porting. Although it's not like I am trying to avoid any changes, but
because this driver worked for us on older kernels (v3.10.9) I am trying
not to touch pieces which IMHO are good enough. Of course I don't mind
suggestions from more experienced developers.
To avoid using do{}while(0) it requires either goto (instead of breaks),
nesting those if()s in one another or a humongous single if(). Neither
looks pretty and the last one is even less readable than
do()while.
--
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics
Attachment:
signature.asc
Description: PGP signature
