On Tue, May 05, 2015 at 12:03:05AM +0800, Fu Wei wrote:
> Hi Guenter,
>
> Great thanks for your feedback, will fix them ASAP.
> Some feedback and comment inline below :
>
[ ... ]
> >> +
> >> +static int sbsa_gwdt_stop(struct watchdog_device *wdd)
> >> +{
> >> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
> >> + struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
> >> +
> >> + u32 status;
> >> +
> >> + mutex_lock(&device_data->lock);
> >> + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
> >> + status &= ~SBSA_GWDT_WCS_EN;
> >> + /* writing WCS will cause an explicit watchdog refresh */
> >> + sbsa_gwdt_cf_write(SBSA_GWDT_WCS, status, wdd);
> >> + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
> >> + mutex_unlock(&device_data->lock);
> >> +
> >> + /* Check if the watchdog was disabled */
> >> + if (status & SBSA_GWDT_WCS_EN)
> >> + return -EACCES;
> >>
> >
> > Same as above.
>
>
> For these two, maybe EPERM?
>
"Operation not permitted" seems just as wrong. Both usually reflect a software
state, not a failure to write to hardware.
The question here really is if this can really happen, and, if it does,
why it happens. In general it is quite uncommon for watchdog drivers to
verify if such writes were successful.
In the given case, if it can in fact happen that the watchdog can not be
stopped under some circumstances, it may make more sense to set the "nowayout"
flag.
If that can not happen, and if you have no rational reason for assuming that the
write can fail, you might just consider removing the read-back code entirely.
[ ... ]
> >> +static long sbsa_gwdt_ioctl(struct watchdog_device *wdd, unsigned int
> >> cmd,
> >> + unsigned long arg)
> >> +{
> >> + struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
> >> + void __user *argp = (void __user *)arg;
> >> + int ret = -ENOIOCTLCMD;
> >> + unsigned int __user *p = argp;
> >> + unsigned int new_value;
> >> +
> >> + /* FIXME: what else do we need here?? */
You'll have to be the person to answer that.
On a higher level, those ioctls need to be moved to the watchdog core,
but that will have to be a separate (unrelated) patch.
Ok, but then you don't really need any of the additional log messages.
> >> + switch (cmd) {
> >> + case WDIOC_SETPRETIMEOUT:
> >> + if (get_user(new_value, p))
> >> + return -EFAULT;
> >> + ret = sbsa_gwdt_set_pretimeout(wdd, new_value);
> >> + break;
> >> + case WDIOC_GETPRETIMEOUT:
> >> + ret = put_user(gwdt->pretimeout, (unsigned int __user
> >> *)arg);
> >> + break;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
> >> +{
> >> + struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
> >> + struct watchdog_device *wdd = &gwdt->wdd;
> >> + u32 status;
> >> + unsigned int left;
> >> +
> >> + pr_debug("SBSA Watchdog: in %s\n", __func__);
> >> +
> >> + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
> >> +
> >> + if (status & SBSA_GWDT_WCS_WS0) {
> >> + pr_crit("SBSA Watchdog WS0 is triggered\n");
> >> + left = sbsa_gwdt_get_timeleft(wdd);
> >> + pr_crit("please try to feed the dog in %ds!\n", left);
> >> +
> >>
> > Pleae no unnecessary noise.
> >
> >
> yes, will simplify it
>
>
> > Really, what do you expect the user to do here ? To be online,
> > watch the kernel log, and do something frantic ?
> >
>
> :-) , that is the legacy code from first version, should be delete or
> simplifed.
>
>
> >
> > + /* FIXME:anything we can do here? */
> >> + panic("SBSA Watchdog pre-timeout");
> >>
> >
> > Isn't this a pretimeout ?
>
>
> yes, in kernel documentaion:
> ----------------------------------
> Pretimeouts:
>
> Some watchdog timers can be set to have a trigger go off before the
> actual time they will reset the system. This can be done with an NMI,
> interrupt, or other mechanism. This allows Linux to record useful
> information (like panic information and kernel coredumps) before it
> resets.
> ---------------------------------
> So I think it makes sense to do a panic here, and if the platform supprot
> kexec/kdump,
> it will be trigger automatically
>
> any suggestion?
>
I don't really see the flags used anywhere, so I don't really understand why
>
> >
> >
> > + }
> >> +
> >> + return IRQ_HANDLED;
> >> +}
> >> +
> >> +static struct watchdog_info sbsa_gwdt_info = {
> >> + .identity = "SBSA Generic Watchdog",
> >> + .options = WDIOF_SETTIMEOUT |
> >> + WDIOF_KEEPALIVEPING |
> >> + WDIOF_MAGICCLOSE |
> >> + WDIOF_PRETIMEOUT |
> >> + WDIOF_CARDRESET,
> >> +};
> >> +
> >> +static struct watchdog_ops sbsa_gwdt_ops = {
> >> + .owner = THIS_MODULE,
> >> + .start = sbsa_gwdt_start,
> >> + .stop = sbsa_gwdt_stop,
> >> + .ping = sbsa_gwdt_keepalive,
> >> + .set_timeout = sbsa_gwdt_set_timeout,
> >> + .ioctl = sbsa_gwdt_ioctl,
> >> + .get_timeleft = sbsa_gwdt_get_timeleft,
> >> +};
> >> +
> >> +
> >> +static int sbsa_gwdt_probe(struct platform_device *pdev)
> >> +{
> >> + struct device *dev = &pdev->dev;
> >> +
> >> + struct sbsa_gwdt *gwdt;
> >> + struct sbsa_gwdt_device_data *device_data;
> >> +
> >> + struct watchdog_device *wdd;
> >> +
> >> + struct resource *res;
> >> + void *rf_base, *cf_base;
> >> + int irq;
> >> + u32 status, flags;
> >> + u32 w_iidr, idr;
> >> +
> >> + int ret = 0;
> >> +
> >> + /*
> >> + * Try to determine the frequency from the cp15 interface
> >> + */
> >> + sbsa_gwdt_rate = arch_timer_get_cntfrq();
> >> + /* Check the system counter frequency. */
> >> + if (!sbsa_gwdt_rate)
> >> + dev_err(dev, "System Counter frequency not available\n");
> >> +
> >> + gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
> >> + if (!gwdt)
> >> + return -ENOMEM;
> >> +
> >> + wdd = &gwdt->wdd;
> >> + wdd->parent = dev;
> >> + device_data = &gwdt->device_data;
> >> + mutex_init(&device_data->lock);
> >> +
> >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >> + "refresh_frame");
> >> + rf_base = devm_ioremap_resource(dev, res);
> >> + if (IS_ERR(rf_base))
> >> + return PTR_ERR(rf_base);
> >> +
> >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >> + "control_frame");
> >> + cf_base = devm_ioremap_resource(dev, res);
> >> + if (IS_ERR(rf_base))
> >> + return PTR_ERR(rf_base);
> >> +
> >> + irq = platform_get_irq_byname(pdev, "ws0");
> >> + if (IS_ERR(irq)) {
> >> + dev_err(dev, "required ws0 interrupt missing\n");
> >> + return PTR_ERR(irq);
> >> + }
> >> +
> >> + res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "ws0");
> >> + if (!res)
> >> + dev_warn(dev, "ws0 interrupt flags missing, ignore\n");
> >> + else
> >> + flags = res->flags;
> >> +
> >>
> > Not really sure why you get ws0 twice. Why don't you just use
> > platform_get_resource_byname() and use it to get the irq number as well ?
> >
>
> yes, I did this before, but some suggestion said it is better to use
> platform_get_irq. so I am doing this way now
> but we can discuss more about this , and will check it again
>
you read them in the first place.
[ ... ]
> >> +
> >> +/* refresh/control frame */
> >> +#define SBSA_GWDT_W_IIDR (0xfcc)
> >> +#define SBSA_GWDT_IDR (0xfd0)
> >>
> >
> > All those ( ) are unnecessary.
>
>
> for this define, I hope we can keep () to prevent someone use these
> uncarefully in some case,
Sorry, that doesn't make sense to me. A constant is a constant
and does not need ( ) around it.
[ ... ]
> >> +
> >> +/**
> >> + * struct sbsa_gwdt_info - SBSA Generic Watchdog device hardware
> >> information
> >> + * @pid: GWDT ProductID
> >> + * @arch_ver: GWDT architecture version
> >> + * @revision: GWDT revision number
> >> + * @vid_bank: GWDT The JEP106 continuation code of the implementer
> >> + * @vid: GWDT The JEP106 identity code of the implementer
> >> + * @refresh_pa: the watchdog refresh frame(PA)
> >> + * @control_pa: the watchdog control frame(PA)
> >> + */
> >> +struct sbsa_gwdt_info {
This structure name is the same as a variable name in the driver.
Please pick another name.
> >> + unsigned int pid;
> >> + unsigned int arch_ver;
> >> + unsigned int revision;
> >> + unsigned int vid_bank;
> >> + unsigned int vid;
> >> + void *refresh_pa;
> >> + void *control_pa;
You fill out some of this data in the driver, but unless I am missing
something I don't see it used, and the code says "for future use".
I don't see the point of doing this. In general, if there is a plan to use
such information at some later time, that is when you should introduce it.
> >> +};
> >> +
> >> +/**
> >> + * struct sbsa_gwdt_device_data - Internal representation of the SBSA
> >> GWDT
> >> + * @refresh_base: VA of the watchdog refresh frame
> >> + * @control_base: VA of the watchdog control frame
> >> + * @irq: Watchdog Timer GSIV (WS0)
> >> + * @flags: Watchdog Timer Flags
> >> + * @dev: Pointer to kernel device structure
> >> + * @info: SBSA Generic Watchdog info structure
> >> + * @lock: GWDT mutex
> >> + */
> >> +struct sbsa_gwdt_device_data {
> >> + void __iomem *refresh_base;
> >> + void __iomem *control_base;
> >> + int irq;
> >> + u32 flags;
Is "flags" used anywhere ?
> >> + struct device *dev;
Is "dev" used anywhere ?
> >> + struct sbsa_gwdt_info info;
> >> + struct mutex lock;
> >> +};
> >> +
> >> +struct sbsa_gwdt {
> >> + struct sbsa_gwdt_device_data device_data;
Can you merge struct device_data into struct sbsa_gwdt ? I don't see the value
of having two structures here, and it just makes the code more complicated.
> >> + struct watchdog_device wdd;
> >> + unsigned int pretimeout; /* seconds */
> >> + unsigned int max_pretimeout;
> >> + u64 wcv_dump;
This is only set but never read. What is it for ?
> >> +#ifdef CONFIG_PM_SLEEP
> >> + u8 pm_status_store;
> >> +#endif
> >> +};
> >> +
> >> +#define to_sbsa_gwdt(e) container_of(e, struct sbsa_gwdt, wdd)
> >> +
> >> +#endif /* _SBSA_GENERIC_WATCHDOG_H_ */
> >>
> >>
> >
>
>
> --
> Best regards,
>
> Fu Wei
> Software Engineer
> Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
> Ph: +86 21 61221326(direct)
> Ph: +86 186 2020 4684 (mobile)
> Room 1512, Regus One Corporate Avenue,Level 15,
> One Corporate Avenue,222 Hubin Road,Huangpu District,
> Shanghai,China 200021